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

[Tune] Add rich output for ray tune progress updates in notebooks #26263

Merged
merged 2 commits into from
Aug 30, 2022

Conversation

peytondmurray
Copy link
Contributor

@peytondmurray peytondmurray commented Jul 1, 2022

These changes are part of a series intended to improve integration with notebooks. This PR modifies the tune progress status shown to the user if tuning is run from a notebook.

Previously, part of the trial progress was reported in an HTML table before; now, all progress is displayed in an organized HTML template.

Summary

  • JupyterNotebookReporter now outputs formatted HTML output rather than a simple string. To do this, I separated out the code which retrieves data to be displayed to the user during a progress update from the code that formats the data as text to be written to the console/log. This data is then used to create html tables if the user is running ray in a notebook.
  • TrialProgressCallback now writes progress updates to a formatted HTML table when a notebook is used. This makes the output of tune.run much more compact and easier to follow.
  • Added a few new HTML templates for output formatting

Here's the tune progress output of a notebook which uses trains a Keras model on the mnist dataset. The trial progress table keeps a sorted list of trials and columns at each update, making it really easy on the eye to keep track of the status of each experiment:

rich_tune_progress

Here's the notebook I used to generate this screenshot: example.zip

Related issue number

See #25591 for more information.

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Tests defined in python/ray/tune/test_progress_reporter.py are passing locally and in CI. The failing tests seem unrelated, with most of the failures related to an issue with starlette trying to import something from typing_extensions that doesn't exist.

Edit: Note the lint failure is actually caused by the CI runner being unable to install pytorch-lightning, and is unrelated.

@peytondmurray peytondmurray force-pushed the jl-tune-progress branch 7 times, most recently from 4336c27 to 0c11ed3 Compare July 8, 2022 19:50
@peytondmurray peytondmurray marked this pull request as ready for review July 8, 2022 23:08
@peytondmurray peytondmurray force-pushed the jl-tune-progress branch 2 times, most recently from a21c151 to 6c83888 Compare July 28, 2022 19:01
@peytondmurray peytondmurray changed the title [WIP] [Tune] Add rich output for ray tune progress updates in notebooks [Tune] Add rich output for ray tune progress updates in notebooks Jul 28, 2022
@bveeramani bveeramani self-assigned this Aug 5, 2022
@bveeramani
Copy link
Member

Hey, thanks for opening this PR. Will review next week

@bveeramani
Copy link
Member

This LGTM at a high level. @krfricke could you take a look once you're back?

@bveeramani bveeramani assigned krfricke and unassigned krfricke Aug 9, 2022
@peytondmurray
Copy link
Contributor Author

@bveeramani @krfricke Gentle reminder about this - if you're too busy to look at this, just let me know and I can ask around for others to take a look.

@krfricke
Copy link
Contributor

Sorry @peytondmurray fo the delay, we were vey busy with Ray 2.0 and the Ray summit this week. That's all over now and we have more bandwidth to look at this - it's on my list for early next week!

Copy link
Contributor

@krfricke krfricke left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for the contribution. We've changed a few function names to private scope, so you'll have to merge latest master to reflect those changes.

For the package structure, could you create a new tune sub-package progress_reporter where we move the old progress_reporter.py and create the template directory within? widgets is a bit random otherwise as we don't have any other widgets.

Also, we should make sure that .j2 files are included in the wheels.

Happy to help with any of the updates if needed, let me know!

@krfricke krfricke added the @external-author-action-required Alternate tag for PRs where the author doesn't have labeling permission. label Aug 29, 2022
@peytondmurray
Copy link
Contributor Author

This looks great, thanks for the contribution. We've changed a few function names to private scope, so you'll have to merge latest master to reflect those changes.

Sounds good. There were no merge conflicts 🥳

For the package structure, could you create a new tune sub-package progress_reporter where we move the old progress_reporter.py and create the template directory within? widgets is a bit random otherwise as we don't have any other widgets.

We actually do have a variety of other templates that are currently being used - see e.g. the worker context and client context. There are also a bunch of instance where we use templates to display Train and AIR config information. I'm still happy to break these templates up if you'd prefer - there are advantages and disadvantages to storing them separately in their own respective subpackages and I could be convinced to do it that way or to store them all in one place - I'm happy to follow whatever other people want. Let me know what you'd prefer.

Also, we should make sure that .j2 files are included in the wheels.

Yes, that detail has been taken care of in a previous commit: https://github.com/ray-project/ray/blob/master/python/setup.py#L199.

Thanks again for the review!

Copy link
Contributor

@krfricke krfricke left a comment

Choose a reason for hiding this comment

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

Sorry, I misread the widgets folder somehow and assumed it was tune/widgets/templates.

Happy with the location for now - thanks again!

@krfricke krfricke merged commit ffe12a5 into ray-project:master Aug 30, 2022
@peytondmurray peytondmurray deleted the jl-tune-progress branch August 31, 2022 00:43
XiaodongLv pushed a commit to XiaodongLv/ray that referenced this pull request Sep 2, 2022
…y-project#26263)

These changes are part of a series intended to improve integration with notebooks. This PR modifies the tune progress status shown to the user if tuning is run from a notebook.

Previously, part of the trial progress was reported in an HTML table before; now, all progress is displayed in an organized HTML template.

Signed-off-by: pdmurray <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@external-author-action-required Alternate tag for PRs where the author doesn't have labeling permission.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants