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.

Sign up for GitHub

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

Jump to bottom

[Tune] Fix docstring failures #32484

Merged
merged 3 commits into from
Feb 14, 2023
Merged

[Tune] Fix docstring failures #32484

merged 3 commits into from
Feb 14, 2023

Conversation

justinvyu
Copy link
Contributor

Why are these changes needed?

This PR fixes the Stopper doctests that are erroring. Previously, it used a tune.Trainable as its trainable, which would error on fit since its methods are not implemented. Also, it was missing some imports.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • 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 :(

@justinvyu
Fix stopper docstring tests
ea00204 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Fix format of from_checkpoint doctest
51c6b49 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Merge branch 'master' of https://github.com/ray-project/ray into air/…
fb7f9da 
…fix_docstrings
justinvyu
justinvyu commented Feb 13, 2023
View reviewed changes
python/ray/tune/stopper/stopper.py
... )
>>> tuner.fit()
== Status ==...
>>> print("[ignore]"); result_grid = tuner.fit() # doctest: +ELLIPSIS
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any other way of doing this? This was the only workaround I could find :(

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess lambda config: {"metric": 1} or something would work, but also happy with this. LMK what you think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, for the training function, I wanted it to actually show the stopper being used -- maybe I'll actually reduce this to 1 second - 5 is a bit excessive.

The print("[ignore]") workaround to catch the Tune output is what I'm wondering about.

krfricke
krfricke approved these changes Feb 13, 2023
View reviewed changes
python/ray/tune/stopper/stopper.py
... )
>>> tuner.fit()
== Status ==...
>>> print("[ignore]"); result_grid = tuner.fit() # doctest: +ELLIPSIS
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess lambda config: {"metric": 1} or something would work, but also happy with this. LMK what you think

@justinvyu
Copy link
Contributor Author

@krfricke Looks like it's good to merge.

@cadedaniel
Copy link
Member

This fixes broken doctests on the 2.3 release branch, let's merge this ASAP.

@krfricke krfricke merged commit 421b527 into ray-project:master Feb 14, 2023
justinvyu added a commit to justinvyu/ray that referenced this pull request Feb 14, 2023
@justinvyu
[Tune] Fix docstring failures (ray-project#32484)
ad3e0f0 
This PR fixes the `Stopper` doctests that are erroring. Previously, it used a `tune.Trainable` as its trainable, which would error on fit since its methods are not implemented. Also, it was missing some imports.

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu justinvyu mentioned this pull request Feb 14, 2023
Merged
7 tasks
justinvyu added a commit that referenced this pull request Feb 14, 2023
@justinvyu
[2.3.0][Tune] Fix docstring failures (#32505)
ccfd8c6 
* [Tune] Fix docstring failures (#32484)

This PR fixes the `Stopper` doctests that are erroring. Previously, it used a `tune.Trainable` as its trainable, which would error on fit since its methods are not implemented. Also, it was missing some imports.

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>

* Make these doctest fixes extra safe

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>

Remove unnecessary imports

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>

---------

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
edoakes pushed a commit to edoakes/ray that referenced this pull request Mar 22, 2023
@justinvyu @edoakes
[Tune] Fix docstring failures (ray-project#32484)
cf2780a 
This PR fixes the `Stopper` doctests that are erroring. Previously, it used a `tune.Trainable` as its trainable, which would error on fit since its methods are not implemented. Also, it was missing some imports.

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krfricke krfricke krfricke approved these changes

@xwjiang2010 xwjiang2010 Awaiting requested review from xwjiang2010

Assignees

@krfricke krfricke

@xwjiang2010 xwjiang2010

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@justinvyu @cadedaniel @krfricke @xwjiang2010