Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log SMC progress via Python logger when progress bar deactivated #7410

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jucor
Copy link
Contributor

@jucor jucor commented Jul 10, 2024

Description

Output terminals that do not support cursor movement ANSI control sequences behave very badly with Rich ProgressBars, and thus need progressbar=false. See discussion in #7233 (comment)

However, even in those cases it is still very worthy to know how the SMC tasks are progressing.

Therefore, in this PR, when progress bars are deactivated, any change to the state of the chains is logged to the default logger, using the exact same state dictionary as used for the progressbars. The fact we only log when a change happens means we do not flood the log.

The use of the logging facility also allows the caller to add timestamps in the logging handlers for a sense of elapsing time, or to filter out these progress updates.

Related Issue

Checklist

  • Checked that the pre-commit linting/style checks pass
  • Included tests that prove the fix is effective or that the new feature works (no test for logging)
  • Added necessary documentation (docstrings and/or example notebooks) (no documentation necessary)
  • If you are a pro: each commit corresponds to a relevant logical change

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

📚 Documentation preview 📚: https://pymc--7410.org.readthedocs.build/en/7410/

Output terminals that do not support cursor movement ANSI control sequences
do not support rich ProgressBars, and thus need progressbar=false. However,
it is still very worthy to know how the SMC tasks are progressing.

Therefore, when progress bars are deactivated, any chance to the
state of the chains is logged to the default logger, using the exact
same state dictionary as used for the progressbars. The fact we only
log when a change happens means we do not flood the log.

The use of the logging facility also allows the called to add timestamps
in the logging handlers for a sense of elapsing time.
@jucor
Copy link
Contributor Author

jucor commented Jul 10, 2024

@fonnesbeck That might be of interest to you :)

Copy link

codecov bot commented Jul 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.18%. Comparing base (1c532f8) to head (08674b3).
Report is 4 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #7410   +/-   ##
=======================================
  Coverage   92.18%   92.18%           
=======================================
  Files         103      103           
  Lines       17258    17270   +12     
=======================================
+ Hits        15909    15921   +12     
  Misses       1349     1349           
Files Coverage Δ
pymc/smc/sampling.py 99.28% <100.00%> (+0.04%) ⬆️

... and 1 file with indirect coverage changes

@jucor
Copy link
Contributor Author

jucor commented Jul 10, 2024

@zaxtax @ricardoV94 Would you please be able to review, given how you were the reviewers for #7233 and thus already know that part of the code? Thank you :)

@ricardoV94
Copy link
Member

I'm still not convinced the change to rich progressbar was a net positive :)

Copy link
Member

@aloctavodia aloctavodia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jucor
Copy link
Contributor Author

jucor commented Aug 11, 2024

@aloctavodia @ricardoV94 @aseyboldt Sorry to nudge, but do you think this could be merged, please?

@zaxtax
Copy link
Contributor

zaxtax commented Aug 11, 2024 via email

@jucor
Copy link
Contributor Author

jucor commented Sep 9, 2024

👋 Any help merging this please? I believe @ricardoV94 requested a review by @aseyboldt on July 12th, but I imagine Adrian is possibly swamped with less trivial PRs :) Any chance to merge this please?

@ricardoV94
Copy link
Member

ricardoV94 commented Sep 9, 2024

Is PyMC log info showing by default? Asking because usually progressbar is disabled when people don't want any output.

@jucor
Copy link
Contributor Author

jucor commented Sep 9, 2024

Good point, thanks for raising it. I believe the progressbar argument means just that: "do you want a progressbar?", and does not say anything about any other way to log.
I've checked the codebase, and found several examples confirming this:

Seems logical: besides the pretty display of a progressbar, any other information is just part of the use of the standard logging facility, and can be muted or filtered using the core library's logging module.

So if anything, we could argue that I should maybe modify this PR to log to the logger even if progressbar is True. However I do not think that such a "logging + progressbar" is warranted in this case, because:

  • the progressbar provides more information than the logging
  • and the logging occurs during the inner loop and thus, if printed, would possibly mess up the rendering of the progressbar (I do not know how gracefully rich handles text that is printed by non-rich calls between two updates to its progressbar)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: SMC progress bars do not display correctly when called from RStudio
4 participants