Skip to content

[tune] Add annotations/set scope for Tune classes#25077

Merged
krfricke merged 11 commits intoray-project:masterfrom
krfricke:tune/annotations
May 25, 2022
Merged

[tune] Add annotations/set scope for Tune classes#25077
krfricke merged 11 commits intoray-project:masterfrom
krfricke:tune/annotations

Conversation

@krfricke
Copy link
Copy Markdown
Contributor

@krfricke krfricke commented May 23, 2022

Why are these changes needed?

This PR adds API annotations or changes the scope of several Ray Tune library classes.

Related issue number

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

Yard1
Yard1 approved these changes May 23, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Looks reasonable to me!

ArturNiederfahrenhorst
ArturNiederfahrenhorst approved these changes May 23, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ArturNiederfahrenhorst ArturNiederfahrenhorst left a comment

Choose a reason for hiding this comment

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

lgtm!

@ericl
Copy link
Copy Markdown
Contributor

ericl commented May 23, 2022

Very nice. Could you also enable the API linter for ray.tune similar to https://github.com/ray-project/ray/pull/25060/files#diff-b52990018d76021fcf0d9c7252371a681b1758c67f20cc7fd5481e3f5e81ee54R94 ?

@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label May 24, 2022
@krfricke
Copy link
Copy Markdown
Contributor Author

krfricke commented May 24, 2022

Added the annotation checker - this checker also checks functions though. I've added some annotations for these as appropriate, but before I annotate/rename the remaining functions I'd like to clarify package structure and annotation policy offline

@krfricke krfricke removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label May 24, 2022
@krfricke
Copy link
Copy Markdown
Contributor Author

krfricke commented May 24, 2022

I've removed it again - I'd like to push forward a package restructuring that we're discussing this week that will then solve the remaining ~85 function annotations.

Can we merge this PR without the check and complete the scope thing with the package refactoring/in a follow up PR?

@ericl
Copy link
Copy Markdown
Contributor

ericl commented May 24, 2022

Sounds reasonable. Some failing tests.

@krfricke krfricke added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label May 24, 2022
Kai Fricke added 2 commits May 24, 2022 23:29
@krfricke krfricke merged commit 67cd984 into ray-project:master May 25, 2022
@krfricke krfricke deleted the tune/annotations branch May 25, 2022 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ArturNiederfahrenhorst ArturNiederfahrenhorst ArturNiederfahrenhorst approved these changes

@Yard1 Yard1 Yard1 approved these changes

@xwjiang2010 xwjiang2010 xwjiang2010 approved these changes

@sven1977 sven1977 Awaiting requested review from sven1977

@gjoliver gjoliver Awaiting requested review from gjoliver

@avnishn avnishn Awaiting requested review from avnishn

@smorad smorad Awaiting requested review from smorad

@maxpumperla maxpumperla Awaiting requested review from maxpumperla

@kouroshHakha kouroshHakha Awaiting requested review from kouroshHakha

@richardliaw richardliaw Awaiting requested review from richardliaw

@ericl ericl Awaiting requested review from ericl

Assignees

@ericl ericl

@richardliaw richardliaw

@Yard1 Yard1

@xwjiang2010 xwjiang2010

Labels

@author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@krfricke @ericl @ArturNiederfahrenhorst @Yard1 @xwjiang2010 @richardliaw