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] Fix TrialTerminationReporter in docs #29254

Merged
merged 3 commits into from
Oct 14, 2022

Conversation

linusbiostat
Copy link
Contributor

Signed-off-by: linusbiostat [email protected]

Why are these changes needed?

To make the documentation code work.

TrialTerminationReporter has an init function. Because extending the CliReporter, it's need to take the same init parameters. I've added these with the super() function.

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 :(

@linusbiostat linusbiostat changed the title Make TrialTerminationReporter working Fix TrialTerminationReporter in docs Oct 12, 2022
@@ -52,9 +52,11 @@ Extending ``CLIReporter`` lets you control reporting frequency. For example:

tuner = tune.Tuner(my_trainable, run_config=air.RunConfig(progress_reporter=ExperimentTerminationReporter()))
results = tuner.fit()


from ray.tune.trial import Trial
Copy link
Member

Choose a reason for hiding this comment

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

is this import necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes seems to be, just updated it to "ray.tune.experiment.trial"

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see where it is used now

Signed-off-by: linusbiostat <[email protected]>
@Yard1
Copy link
Member

Yard1 commented Oct 12, 2022

Awesome, thanks - one nit, can we move the Trial import to the top of the code block and leave an empty line after it?

Copy link
Member

@Yard1 Yard1 left a comment

Choose a reason for hiding this comment

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

(please see above)

We should also move this to testable doc code, but we can do that in a followup.

@Yard1 Yard1 self-assigned this Oct 13, 2022
Signed-off-by: Antoni Baum <[email protected]>
@richardliaw richardliaw changed the title Fix TrialTerminationReporter in docs [tune] Fix TrialTerminationReporter in docs Oct 14, 2022
@richardliaw richardliaw merged commit f86b141 into ray-project:master Oct 14, 2022
WeichenXu123 pushed a commit to WeichenXu123/ray that referenced this pull request Dec 19, 2022
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