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

YTEP-0039: Rich Terminal User Interface #18

Merged
merged 3 commits into from
Feb 5, 2022

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Mar 3, 2021

TLDR: I propose replacing our logging handler with rich's version for it, and expose other potential applications for this library in yt.

Much more detail awaits in yt-project/yt#3106, but this YTEP specifies the new configuration parameters.

Here are the embedded illustrations to help visualise the proposed change for yt's logger

From a notebook

Legacy

ytep-0039_color_notebook_log_legacy

Rich

ytep-0039_color_notebook_log_rich

From a terminal

Legacy

ytep-0039_color_shell_log_legacy

Rich

ytep-0039_color_shell_log_rich

@matthewturk
Copy link
Member

I like this a lot. Need to think about what default is for next release -- maybe gradual phase-in?

@neutrinoceros
Copy link
Member Author

neutrinoceros commented Mar 3, 2021

Yeah it could be added in time for 4.0 as an experimental feature and only made the default in 4.2 😃

@neutrinoceros neutrinoceros force-pushed the ytep-rich branch 7 times, most recently from 8d225f9 to 286823c Compare March 6, 2021 23:13
@neutrinoceros neutrinoceros marked this pull request as ready for review March 7, 2021 00:00
@neutrinoceros neutrinoceros force-pushed the ytep-rich branch 3 times, most recently from de33003 to 8616b6c Compare March 9, 2021 08:26
Copy link
Member

@brittonsmith brittonsmith left a comment

Choose a reason for hiding this comment

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

Ok, I like this quite a bit and it doesn't seem to be as invasive a change as I originally perceived. I'm quite intrigued by the idea of multiple concurrent progress bars mixed with logging messages. This could be neat for some parallel operations. I'm happy to accept this and for the corresponding yt PRs to go ahead.

@neutrinoceros
Copy link
Member Author

I am tempted to drop this YTEP entirely for the following reasons:

  • rich progress bars are not debugger friendly and it's not clear that they ever will be ([BUG] debugger inside progress bar Textualize/rich#1465)
  • both patches #3106 and #3614 have been rotting for month now and I don't think they are worth the maintenance time they've cost me already
  • the proposed enhancements are superficial, add dependencies, and do not add clear value for users
  • I've been maintaining a very small project for a couple month now, who's sole dependency is rich, and report that upstream is far less stable that I would have hoped. For instance today CI is broken there because rich uses poetry as a build backend, but the latest version of setuptools breaks poetry. Yt doesn't currently depend on poetry, even indirectly, and I don't think it's worth the additional chances of breaking CI from upstream changes (something we see a lot in yt already).

@matthewturk, I suppose I should I mark this YTEP as "rejected", then we can merge this PR for posterity ?

@matthewturk
Copy link
Member

Not yet? Can I read carefully before you do anything?

@neutrinoceros
Copy link
Member Author

As you wish, but I TBH I don't think it's worth any more of anyone's time.

@neutrinoceros
Copy link
Member Author

Sorry Matt I just can't keep circling back to this. I've marked the YTEP as withdrawn, and I'll close my associated PRs on the yt side.

@neutrinoceros
Copy link
Member Author

I'll just merge myself here, as I can't think of any other route to go with a withdrawn proposal. The document only has a historical value now so there's no reason to change it anymore. I hope that's okay with everyone.

@neutrinoceros neutrinoceros merged commit 77816fc into yt-project:master Feb 5, 2022
@neutrinoceros neutrinoceros deleted the ytep-rich branch February 5, 2022 13:00
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.

3 participants