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

[py312] Skip tests+examples for Tune dependencies that do not support py312 #46645

Closed
wants to merge 17 commits into from
Closed

[py312] Skip tests+examples for Tune dependencies that do not support py312 #46645

wants to merge 17 commits into from

Conversation

justinvyu
Copy link
Contributor

Why are these changes needed?

This PR addresses the ML CI failures, aside from the horovod and TF issues:

  • Libraries with a latest version that doesn’t support py312 (HEBO, Aim) will not run tests on py312. These should be removed eventually when py312 becomes the lower bound version, if the library is not updated by then.
  • BOHB's main dependency is not maintained anymore, so the tests here are also skipped, and this dependency should be removed in the future.
  • Drive-by removal of tune-sklearn dependency, as this is no longer needed.

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 added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • 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
remove tune multinode sync test
206a6fe 
Signed-off-by: Justin Yu <[email protected]>
@justinvyu
upgrade comet-ml
bda65bd 
Signed-off-by: Justin Yu <[email protected]>
@justinvyu
remove tune-sklearn
e2f8b51 
Signed-off-by: Justin Yu <[email protected]>
@justinvyu
skip hebo tests in >=py312
67d604b 
Signed-off-by: Justin Yu <[email protected]>
@justinvyu
remove aim usage in tests
a06f31b 
Signed-off-by: Justin Yu <[email protected]>
@justinvyu
update bohb requirements
7644262 
Signed-off-by: Justin Yu <[email protected]>
@justinvyu
skip bohb tests
cabce6f 
Signed-off-by: Justin Yu <[email protected]>
@justinvyu
reenable bohb example for py<312
bf8ba34 
Signed-off-by: Justin Yu <[email protected]>
@justinvyu
fix lint
117a9d8 
Signed-off-by: Justin Yu <[email protected]>
matthewdeng
matthewdeng reviewed Jul 16, 2024
View reviewed changes
python/ray/tune/examples/bohb_example.py

import pytest

if sys.version_info >= (3, 12):
Copy link
Contributor

Choose a reason for hiding this comment

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

What version of Python does our CI use? IIRC we used the minimum supported version, but I might be wrong.

Reason I ask is if we don't use 3.12, I'm worried this will get missed in the future, and it might be better to let it explicitly fail and we can make a decision on what to do with the integration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@can-anyscale Is CI using py39?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@can-anyscale How often does CI run with py312?

Is it ok if these tests explicitly fail for 312? That way, when py312 becomes the minimum version, we can be aware of what code is broken so it can be fully removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is CI using py39?

CI uses py39 yes

How often does CI run with py312?

You decide; currently we only run tests with py39 versions; historically for other python versions we do a one-fix effort during upgrade but do not run tests continuously

Is it ok if these tests explicitly fail for 312?

We can choose to intentionally NOT run/fix it for python 3.12 now and let it fail explicitly when CI turns py39 to py312

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can choose to intentionally NOT run/fix it for python 3.12 now and let it fail explicitly when CI turns py39 to py312

Explicitly failing on py312 makes sense to me!

Copy link
Collaborator

Choose a reason for hiding this comment

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

sweet, you can just note in https://docs.google.com/spreadsheets/d/1VIpZdrp24CiWkI13EE__REIFlVn0lDiSBLuCN5cyOP8/edit?gid=2025695384#gid=2025695384 that we intentionally ignore the tests for python 3.12 and mark it as green, thankks

python/ray/tune/tests/test_convergence.py Outdated
@@ -79,6 +80,7 @@ def testConvergenceBayesOpt(self):
assert len(analysis.trials) < 50
assert math.isclose(analysis.best_config["x"], 0, abs_tol=1e-5)

@pytest.mark.skipif(sys.version_info >= (3, 12))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Comment why it's not supported.

python/ray/tune/tests/test_multinode_sync.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this deletion related to 3.12?

Copy link
Contributor Author

@justinvyu justinvyu Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

there was something in this test that was complaining about an external py39 docker image != py312. This test also covers some ray core level functionality which should not be in Train

python/requirements/ml/tune-requirements.txt Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to update the compiled requirements as well?

@anyscalesam anyscalesam added P1 Issue that should be fixed within a few weeks train Ray Train Related Issue labels Jul 16, 2024
can-anyscale
can-anyscale reviewed Jul 16, 2024
View reviewed changes
Copy link
Collaborator

@can-anyscale can-anyscale left a comment

Choose a reason for hiding this comment

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

@justinvyu
Copy link
Contributor Author

closed in favor of #46761

@justinvyu justinvyu closed this Jul 23, 2024
@justinvyu justinvyu deleted the py312/ml-fixes branch July 23, 2024 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewdeng matthewdeng matthewdeng left review comments

@can-anyscale can-anyscale can-anyscale left review comments

@hongpeng-guo hongpeng-guo Awaiting requested review from hongpeng-guo hongpeng-guo 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

@matthewdeng matthewdeng

@can-anyscale can-anyscale

Labels
P1 Issue that should be fixed within a few weeks train Ray Train Related Issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@justinvyu @matthewdeng @can-anyscale @anyscalesam