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

[doc][ml] add missing tune public API references + api policy lint checker #47138

Merged
merged 1 commit into from
Aug 27, 2024
Merged

[doc][ml] add missing tune public API references + api policy lint checker #47138

merged 1 commit into from
Aug 27, 2024

Conversation

can-anyscale
Copy link
Collaborator

@can-anyscale can-anyscale commented Aug 14, 2024 •
edited
Loading

  • Add missing tune public API references
  • Turn on API policy int checker so all public API needs to be documented going forward

Test:

  • CI

@can-anyscale can-anyscale force-pushed the can-dml02 branch 3 times, most recently from 507cdc8 to a0eb65f Compare August 14, 2024 21:03
can-anyscale
can-anyscale commented Aug 14, 2024
View reviewed changes
python/ray/air/config.py
@@ -650,7 +649,9 @@ class RunConfig:
verbose: Optional[Union[int, "AirVerbosity", "Verbosity"]] = None
stop: Optional[Union[Mapping, "Stopper", Callable[[str, Mapping], bool]]] = None
callbacks: Optional[List["Callback"]] = None
progress_reporter: Optional["ProgressReporter"] = None
progress_reporter: Optional[
"ray.tune.progress_reporter.ProgressReporter" # noqa: F821
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

need to use full path now since the name ProgressReporter is ambiguous to sphinx, with the addition of ray.tune.experimental.output.ProgressReporter

Copy link

Choose a reason for hiding this comment

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

@can-anyscale @justinvyu is this just a hack to get a correct link in the docs? Could this be done in a different way that doesn't break the typing?

For reference see omni-us/jsonargparse#609 (comment). Before I found this pull request I also asked the question here https://discuss.ray.io/t/why-the-invalid-type-for-progress-reporter-of-ray-train-runconfig/20178

Copy link

Choose a reason for hiding this comment

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

Maybe this #48439?

@can-anyscale can-anyscale marked this pull request as ready for review August 14, 2024 21:53
justinvyu
justinvyu reviewed Aug 19, 2024
View reviewed changes
ci/ray_ci/doc/cmd_check_api_discrepancy.py Outdated
Comment on lines 53 to 59
"white_list_apis": {
# Already documented as ray.tune.search.ConcurrencyLimiter
"ray.tune.search.searcher.ConcurrencyLimiter",
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

will every class with an alias need this whitelist?

ex: ray.train.torch.torch_trainer.TorchTrainer vs. ray.train.torch.TorchTrainer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not that every class with an alias needs a whitelist, but for some reason, this particular class has two versions:

we can document both but not sure why they look the same

doc/source/tune/api/stoppers.rst Outdated
Comment on lines 49 to 50
function_stopper.FunctionStopper
noop.NoopStopper
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add the ~ in front?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

totally

doc/source/tune/api/trainable.rst Outdated
@@ -248,4 +248,5 @@ Tune Trainable Debugging Utilities

tune.utils.diagnose_serialization
tune.utils.validate_save_restore

tune.utils.util.validate_warmstart
tune.utils.log.Verbosity
Copy link
Contributor

Choose a reason for hiding this comment

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

This is replaced by AirVerbosity, but it's also not officially deprecated 😞 . Can you remove + whitelist this for now and add a deprecation TODO(ml-team)?

The whole "tune output" module needs to be cleaned up since it's in an in-between state at the moment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

totally

Copy link
Contributor

Choose a reason for hiding this comment

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

oh can we remove the Verbosity from this list then? and add it to the whitelist?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

w00t yes good catch

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@justinvyu removed, thankks

Base automatically changed from can-dml01 to master August 19, 2024 20:34
@can-anyscale can-anyscale force-pushed the can-dml02 branch 2 times, most recently from eea77a7 to 4aae96d Compare August 19, 2024 20:41
can-anyscale
can-anyscale commented Aug 19, 2024
View reviewed changes
ci/ray_ci/doc/cmd_check_api_discrepancy.py
"head_doc_file": "doc/source/tune/api/api.rst",
"white_list_apis": {
# Already documented as ray.tune.search.ConcurrencyLimiter
"ray.tune.search.searcher.ConcurrencyLimiter",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for some reason, this particular class has two versions:

we can document both but not sure why they look the same

@can-anyscale
Copy link
Collaborator Author

address @justinvyu's comments

@can-anyscale
Copy link
Collaborator Author

test failure is unrelated

justinvyu
justinvyu approved these changes Aug 27, 2024
View reviewed changes
Copy link
Contributor

@justinvyu justinvyu left a comment

Choose a reason for hiding this comment

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

Thanks!

@can-anyscale can-anyscale merged commit e32eb38 into master Aug 27, 2024
4 of 5 checks passed
@can-anyscale can-anyscale deleted the can-dml02 branch August 27, 2024 16:42
@mauvilsa mauvilsa mentioned this pull request Oct 30, 2024
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mauvilsa mauvilsa mauvilsa left review comments

@justinvyu justinvyu justinvyu approved these changes

@aslonnie aslonnie aslonnie approved these changes

@hongpeng-guo hongpeng-guo Awaiting requested review from hongpeng-guo hongpeng-guo is a code owner

@matthewdeng matthewdeng Awaiting requested review from matthewdeng matthewdeng is a code owner

@raulchen raulchen Awaiting requested review from raulchen raulchen is a code owner

@woshiyyya woshiyyya Awaiting requested review from woshiyyya woshiyyya is a code owner

Assignees

@justinvyu justinvyu

@matthewdeng matthewdeng

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@can-anyscale @justinvyu @mauvilsa @aslonnie @matthewdeng