Skip to content

Add marimo progress bar for pymc sampler #7883

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

williambdean
Copy link
Contributor

@williambdean williambdean commented Jul 31, 2025

Description

The rich progress bars don't work well in marimo notebooks. Here is a basic implementation using their progress bar component to
track the pymc sampler.

An implementation note: tasks being attached to the progress bar and progress bar manager is a bit confusing, but I tried to keep as lean as possible to get to work.

Screen.Recording.2025-07-30.at.10.56.29.PM.mov

Related Issue

  • Closes #
  • Related to #

Checklist

Type of change

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

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

@williambdean williambdean requested a review from ericmjl July 31, 2025 03:07
@williambdean williambdean requested a review from ricardoV94 July 31, 2025 03:12
Copy link

codecov bot commented Jul 31, 2025

Codecov Report

❌ Patch coverage is 63.88889% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.83%. Comparing base (58b49f2) to head (886b584).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
pymc/progress_bar.py 63.88% 26 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7883      +/-   ##
==========================================
- Coverage   92.94%   92.83%   -0.12%     
==========================================
  Files         116      116              
  Lines       18845    18900      +55     
==========================================
+ Hits        17516    17545      +29     
- Misses       1329     1355      +26     
Files with missing lines Coverage Δ
pymc/progress_bar.py 82.52% <63.88%> (-10.86%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ricardoV94
Copy link
Member

Don't love this. Is there any plan for rich to work with marimo?

@williambdean
Copy link
Contributor Author

Don't love this. Is there any plan for rich to work with marimo?

rich shut it down
Textualize/rich#3750

@ricardoV94
Copy link
Member

Also we use CustomProgress directly in other places (.e.g, pm.compute_log_likelihood, sample_posterior_predictive)

Comment on lines +224 to +236
def compute_draw_speed(elapsed, draws):
speed = draws / max(elapsed, 1e-6)

if speed > 1 or speed == 0:
unit = "draws/s"
else:
unit = "s/draws"
speed = 1 / speed

return speed, unit


def create_rich_progress_bar(full_stats, step_columns, progressbar, progressbar_theme):
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why these aren't methods anymore? It's not like they are used outside of ProgressBarManager (compute_draw_speed was static method anyway)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ProgressBarManager seemed to be doing too much. The second one was specific to the rich implementation

Copy link
Member

Choose a reason for hiding this comment

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

This still seems like a function that would ever only be called by ProgressBarManager, the inputs are way too specific

Copy link
Member

Choose a reason for hiding this comment

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

And the object that is created is also called very specifically by ProgressBarManager

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ideally it would be passed in as many of the init kwargs of the ProgressBarManager are specific to this type of ProgressBar

Copy link
Member

Choose a reason for hiding this comment

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

What would be passed in? I was just arguing against this function being moved out of the class, because it seems tightly coupled to it anyway

@williambdean
Copy link
Contributor Author

Also we use CustomProgress directly in other places (.e.g, pm.compute_log_likelihood, sample_posterior_predictive)

CustomProgress isn't changed and would still work. The ProgressBarManager now excepts other ProgressBar protocols

Comment on lines +306 to +320
def update(self, task_id, **kwargs):
"""Update the task with the given ID with the provided keyword arguments."""
if self.bar.progress.current >= cast(int, self.bar.progress.total):
return

self.divergences[task_id.chain_idx] = kwargs.get("divergences", 0)

total_divergences = sum(self.divergences.values())

update_kwargs = {}
if total_divergences > 0:
word = "draws" if total_divergences > 1 else "draw"
update_kwargs["subtitle"] = f"{total_divergences} diverging {word}"

self.bar.progress.update(**update_kwargs)
Copy link
Member

@ricardoV94 ricardoV94 Jul 31, 2025

Choose a reason for hiding this comment

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

We changed the logic to not be obsessed with NUTS.

There is now a failing variable that the step methods report back, and that's passed to update to indicate sampling is failing. Also each step decides what is a meaningful stat to report.

This reverts back to over-specializing for nuts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does failing report the number of divergences? I can remove this logic and just not have a subtitle which would be specific to NUTS

Copy link
Member

Choose a reason for hiding this comment

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

Failing is just binary, we use for coloring. The relevant summary stats are reported as part of all_step_stats, we don't treat them specially.

**all_step_stats,

@ricardoV94
Copy link
Member

Also we use CustomProgress directly in other places (.e.g, pm.compute_log_likelihood, sample_posterior_predictive)

CustomProgress isn't changed and would still work. The ProgressBarManager now excepts other ProgressBar protocols

Other methods don't use ProgressBarManager, but CustomProgress, which is a rich object

@ricardoV94
Copy link
Member

Such as

pymc/pymc/backends/arviz.py

Lines 686 to 688 in 011fb35

with CustomProgress(
console=Console(theme=progressbar_theme), disable=not progressbar
) as progress:

@williambdean
Copy link
Contributor Author

Such as

pymc/pymc/backends/arviz.py

Lines 686 to 688 in 011fb35

with CustomProgress(
console=Console(theme=progressbar_theme), disable=not progressbar
) as progress:

That is using rich (inherited) CustomProgress directly... completely coupled with rich and not the ProgressBarManager absraction.

I was focusing on the NUTS sampler progress bar specifically but can broaden to the where CustomProgress is used if that's your concern. However, those just won't work in marimo as they use rich.

@ricardoV94
Copy link
Member

Furthermore, is there a way to have multiple progress_bars, color them manually? Fine if not, but keeping things similar between backends may help with code complexity

@williambdean
Copy link
Contributor Author

Furthermore, is there a way to have multiple progress_bars, color them manually? Fine if not, but keeping things similar between backends may help with code complexity

You can, but it takes but a bit of real estate so I opted for the single one to be simple. I'd think that'd be determined by the add_tasks method from the ProgressBar protocol

@ricardoV94
Copy link
Member

I was focusing on the NUTS sampler progress bar specifically but can broaden to the where CustomProgress is used if that's your concern. However, those just won't work in marimo as they use rich.

Yeah and that's a valid strategy. I'm raising it in case that also solving the CustomProgress fixes more cases / provides a solution that is different than the one you took here specifically for the ProgressBarManager

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

Successfully merging this pull request may close these issues.

2 participants