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

[Ray 2.9.0 Release] Update Ray versions from 2.8.0 to 2.9.0 #1770

Merged
merged 9 commits into from
Dec 28, 2023

Conversation

architkulkarni
Copy link
Contributor

@architkulkarni architkulkarni commented Dec 26, 2023

Why are these changes needed?

As part of the 2.9.0 release, this PR updates the latest Ray version used in the KubeRay repo to 2.9.0. Example PR: #1678

Related issue number

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

Signed-off-by: Archit Kulkarni <architkulkarni@users.noreply.github.com>
…ray-2.9.0-update

Signed-off-by: Archit Kulkarni <architkulkarni@users.noreply.github.com>
Signed-off-by: Archit Kulkarni <architkulkarni@users.noreply.github.com>
Signed-off-by: Archit Kulkarni <architkulkarni@users.noreply.github.com>
@@ -151,9 +151,9 @@ def test_ray_serve(self):
self.fail(f"Fail to execute test_ray_serve_2.py. The exit code is {exit_code}.")

@unittest.skipIf(
ray_version == '2.8.0' or ray_version == 'nightly',
'test_detached_actor is too flaky with Ray 2.8.0 and Ray nightly.'
'Therefore, skip it until https://github.com/ray-project/ray/issues/41343 is solved.'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the test that is unskipped in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Why does Compatibility Test 2.8.0 still fail with test_detached_actor? It should be skipped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how this is possible, still investigating. According to the code it looks like we are still skipping it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect it's because the decorators are evaluated when the function is defined, not at the first the time the function is executed. So when it's evaluated, the global variable ray_version is still set to its initial value of 2.9.0, so it doesn't get skipped.

Before this PR the initial value was 2.8.0, so the test was actually being skipped for all compatibility tests (2.5, 2.6, 2.7, 2.8, nightly). I'll just fix it directly in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! 3b0718d

with:
ray_version: 2.5.0

test-compatibility-2_6_3:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously we were testing 2.5.0, 2.6.3, 2.7.0, and 2.8.0.

In this PR, we only test 2.7.0, 2.8.0, and 2.9.0.

ray-operator/controllers/ray/rayservice_controller.go Outdated Show resolved Hide resolved
@@ -151,9 +151,9 @@ def test_ray_serve(self):
self.fail(f"Fail to execute test_ray_serve_2.py. The exit code is {exit_code}.")

@unittest.skipIf(
ray_version == '2.8.0' or ray_version == 'nightly',
'test_detached_actor is too flaky with Ray 2.8.0 and Ray nightly.'
'Therefore, skip it until https://github.com/ray-project/ray/issues/41343 is solved.'
Copy link
Member

Choose a reason for hiding this comment

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

Why does Compatibility Test 2.8.0 still fail with test_detached_actor? It should be skipped.

Signed-off-by: Archit Kulkarni <architkulkarni@users.noreply.github.com>
…ray-2.9.0-update

Signed-off-by: Archit Kulkarni <architkulkarni@users.noreply.github.com>
Signed-off-by: Archit Kulkarni <architkulkarni@users.noreply.github.com>
Signed-off-by: Archit Kulkarni <architkulkarni@users.noreply.github.com>
@@ -29,7 +29,7 @@
)

# Default Ray version
ray_version = '2.8.0'
ray_version = '2.9.0'
Copy link
Member

Choose a reason for hiding this comment

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

I found that @unittest.skipIf uses ray_version with the 2.9.0. We may remove ray_version here, and only initialize the global variable in __main__.

Copy link
Member

Choose a reason for hiding this comment

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

The interesting thing is that when I print ray_version in setUpClass, the value is 2.8.0.

@kevin85421
Copy link
Member

I am not a big fan of checking whether to skip tests at runtime. I have opened an issue to track the progress: #1773. This is a good first issue for contributors.

Signed-off-by: cindyz <cindyz@anyscale.com>
@architkulkarni architkulkarni merged commit 827814c into ray-project:master Dec 28, 2023
24 checks passed
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.

None yet

2 participants