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

make progrgress_bar param accept a dict #778

Merged
merged 2 commits into from
Feb 8, 2024

Conversation

wsuzume
Copy link
Contributor

@wsuzume wsuzume commented Feb 6, 2024

Fixes #777 (Oh, I unintendedly hit the jackpot🎰)

What does this PR do?

This pull request is an extension that allows passing arguments to tqdm by enabling the progress_bar parameter of execute_notebook to accept a dict.

This pull request maintains compatibility with the current implementation, though it might make the conditional branching a bit messy...

I believe there could be discussions on how to actually enable such specification, as follows.

            # Ensure at least unit and desc.
            # Conversely, it becomes impossible to remove unit and desc.
            elif isinstance(progress_bar, dict):
                _progress_bar = { "unit": "cell", "desc": "Executing" }
                _progress_bar.update(progress_bar)
                self.pbar = tqdm(total=len(self.nb.cells), **_progress_bar)
            # Simple and accepts any dictionary.
            # However, it's not intuitive as an empty dictionary is precluded by `if progress_bar:`.
            elif isinstance(progress_bar, dict):
                self.pbar = tqdm(total=len(self.nb.cells), **progress_bar)
        # It becomes possible to accept an empty dictionary,
        # but the implementation becomes slightly complicated.
        if isinstance(progress_bar, bool):
            if progress_bar:
                # lazy import due to implicit slow ipython import
                from tqdm.auto import tqdm
                ...
        elif isinstance(progress_bar, dict):
            # lazy import due to implicit slow ipython import
            from tqdm.auto import tqdm
            ...

Copy link

codecov bot commented Feb 6, 2024

Codecov Report

Merging #778 (385be4e) into main (cb2eb37) will decrease coverage by 0.28%.
Report is 3 commits behind head on main.
The diff coverage is 28.57%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #778      +/-   ##
==========================================
- Coverage   91.54%   91.27%   -0.28%     
==========================================
  Files          17       17              
  Lines        1621     1627       +6     
==========================================
+ Hits         1484     1485       +1     
- Misses        137      142       +5     

Copy link
Member

@MSeal MSeal left a comment

Choose a reason for hiding this comment

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

This seems like a great improvement for the control plane of progress bars. Thanks for exploring and adding it!

@MSeal MSeal merged commit fc6d363 into nteract:main Feb 8, 2024
12 of 13 checks passed
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.

Make the parameters of progress_bar specifiable by a dictionary
2 participants