-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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 RichProgressBar more flexible with Rich.Console #10875
Conversation
when force_terminal is True, the progress_bar can be logged in nohup output file
@kaushikb11 Hi, how do you think about this pull request |
Hi @quancs, thanks for doing the PR! I think it would be better to have def __init__(
self,
refresh_rate: int = 1,
leave: bool = False,
theme: RichProgressBarTheme = RichProgressBarTheme(),
console_kwargs: Optional[Dict[str, Any]] = None,
) -> None: |
Is this compatible with CLI? @kaushikb11 |
Co-authored-by: thomas chaton <[email protected]>
Co-authored-by: thomas chaton <[email protected]>
Co-authored-by: thomas chaton <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR @quancs!
Would've been nicer if we could pass the console as an argument to the rich progress callback, however because of the pickling issue with the Console this would have to be a workaround
@@ -324,7 +327,7 @@ def __getstate__(self): | |||
|
|||
def __setstate__(self, state): | |||
self.__dict__ = state | |||
state["_console"] = Console() | |||
self._console = Console(**self._console_kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after #10896 _init_progress
should run in the spawned processes already. We can remove this setstate method entirely (in a new PR) if you agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this
console_kwargs: Optional[Dict[str, Any], Console] = None,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be up for that, but we could add that in a separate PR if you'd prefer :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. So, let's merge this PR?
Or do you have any other thoughts? @awaelchli
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, are you interested to make a follow up with my proposed change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'd like to. Should I propose a issue first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my proposal here, no, you can just send a PR: #10875 (comment)
I didn't fully understand what @SeanNaren said but yeah, probably fine to send a PR without issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
The
Console
insideRichProgressBar
is fixed to have its default paramters, which leads to some problems: for example, when force_terminal is True, the progress_bar can be logged in nohup output file. Besides, that is very inflexible for people who want to make some other settings.What does this PR do?
Add
**kwargs
toRichProgressBar
's init args for initializing its innerConsole
, so that #10876 can be fixed by passingforce_terminal=True
Fixes #10876
Does your PR introduce any breaking changes? If yes, please list them.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃