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

[AIR/train] Move predictors to ray.train #25769

Merged
merged 27 commits into from
Jun 16, 2022
Merged

[AIR/train] Move predictors to ray.train #25769

merged 27 commits into from
Jun 16, 2022

Conversation

Yard1
Copy link
Member

@Yard1 Yard1 commented Jun 14, 2022 •
edited
Loading

Why are these changes needed?

Moves predictors from ray.air to ray.train to converge upon the agreed package structure. There are no changes aside from moving the packages and tests to their new home, and fixing lint issues.

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 added this to the Ray AIR milestone Jun 14, 2022
@Yard1 Yard1 requested review from ericl and scv119 as code owners June 14, 2022 17:03
@Yard1
Copy link
Member Author

Yard1 commented Jun 14, 2022

A lot of changes are due to the new linting sorting the imports

@Yard1 Yard1 marked this pull request as draft June 15, 2022 08:37
@Yard1 Yard1 marked this pull request as ready for review June 15, 2022 08:42
amogkam
amogkam reviewed Jun 15, 2022
View reviewed changes
Copy link
Contributor

@amogkam amogkam left a comment

Choose a reason for hiding this comment

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

Thanks @Yard1! Just left a couple small comments. Can we also move sklearn_utils.py to the sklearn directory but make the functions private? Just to have all the sklearn related things in one directory.

python/ray/data/dataset.py Outdated Show resolved Hide resolved
python/ray/serve/model_wrappers.py Outdated Show resolved Hide resolved
@Yard1 Yard1 added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Jun 15, 2022
richardliaw
richardliaw approved these changes Jun 15, 2022
View reviewed changes
Copy link
Contributor

@richardliaw richardliaw left a comment

Choose a reason for hiding this comment

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

stamping

clarkzinzow
clarkzinzow approved these changes Jun 15, 2022
View reviewed changes
Copy link
Contributor

@clarkzinzow clarkzinzow left a comment

Choose a reason for hiding this comment

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

Stamping under the assumption that the data changes are essentially a no-op, wasn't able to detect any changes other than the import re-ordering.

@richardliaw richardliaw merged commit 91dd360 into ray-project:master Jun 16, 2022
@Yard1 Yard1 deleted the move_predictors_to_ray_train branch June 16, 2022 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amogkam amogkam amogkam left review comments

@clarkzinzow clarkzinzow clarkzinzow approved these changes

@richardliaw richardliaw richardliaw approved these changes

@ericl ericl Awaiting requested review from ericl

@scv119 scv119 Awaiting requested review from scv119

@jjyao jjyao Awaiting requested review from jjyao

@jianoaix jianoaix Awaiting requested review from jianoaix

@maxpumperla maxpumperla Awaiting requested review from maxpumperla

@pcmoritz pcmoritz Awaiting requested review from pcmoritz

@edoakes edoakes Awaiting requested review from edoakes

@simon-mo simon-mo Awaiting requested review from simon-mo

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

Assignees

@matthewdeng matthewdeng

@amogkam amogkam

Labels
tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Milestone
Ray AIR
Development

Successfully merging this pull request may close these issues.
5 participants
@Yard1 @richardliaw @clarkzinzow @amogkam @matthewdeng