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

[ci][byod] validate that correct ray is installed #36794

Merged
merged 1 commit into from
Jun 28, 2023

Conversation

can-anyscale
Copy link
Collaborator

@can-anyscale can-anyscale commented Jun 24, 2023

Why are these changes needed?

  • Validate that ray is at the correct commit under test after building BYOD image. This is a feature that exists in the old way of running release tests, I just port the validation check here.

Checks

@can-anyscale can-anyscale force-pushed the can-validate-wheel branch 2 times, most recently from a8ec324 to c0200d0 Compare June 24, 2023 16:54
@can-anyscale can-anyscale changed the title Validate correct ray is installed [ci][byod] validate that correct ray is installed Jun 26, 2023
@can-anyscale can-anyscale marked this pull request as ready for review June 26, 2023 17:38
@can-anyscale can-anyscale requested a review from a team as a code owner June 26, 2023 17:38
@can-anyscale can-anyscale added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Jun 26, 2023
Copy link
Collaborator

@aslonnie aslonnie left a comment

Choose a reason for hiding this comment

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

I am actually thinking about removing this ray.__commit__ thing in non-final releases. this is invalidating all the bazel build and test caches on every build that requires the wheel.

release/ray_release/byod/byod.Dockerfile Outdated Show resolved Hide resolved
@can-anyscale
Copy link
Collaborator Author

can-anyscale commented Jun 26, 2023

Lol interesting, is this because the file that contains the commit hash has so many dependencies that a change to it cause the whole graph to be rebuilt? Can it be move outside of the dependency graph?

@aslonnie
Copy link
Collaborator

is this because the file that contains the commit hash has so many dependencies that a change to it cause the whole graph to be rebuilt? Can it be move outside of the dependency graph?

there are changes that are not affecting the wheel and does not need a wheel rebuild, but the __commit__ field is guaranteeing that a wheel will always get rebuilt for every commit.

@can-anyscale can-anyscale removed the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Jun 27, 2023
@can-anyscale can-anyscale requested a review from aslonnie June 27, 2023 20:17
@can-anyscale
Copy link
Collaborator Author

@aslonnie : have we decided to support this going forward, but do it in a way that doesn't break the cache, or we haven't decided what to do here yet?

@aslonnie
Copy link
Collaborator

have we decided to support this going forward, but do it in a way that doesn't break the cache, or we haven't decided what to do here yet?

I think having the check here for now is fine, but can we run the test not in the docker file? one can either use docker run and print out the commit and check, or docker create+docker cp to copy out the __commit__.py file, or even use a library to extract the file from the container image without creating a container.

Dockerfile is supposedly for build time, where the commit check is more like a test I think.

Copy link
Collaborator

@aslonnie aslonnie left a comment

Choose a reason for hiding this comment

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

as in the other comment, can we have a test outside the dockerfile but after the build?

@can-anyscale
Copy link
Collaborator Author

We can run them as part of the test execution yeah. I like that, in that way, the issue will also be bisectable.

@can-anyscale can-anyscale force-pushed the can-validate-wheel branch 4 times, most recently from 159c88b to 07986bf Compare June 28, 2023 01:15
Signed-off-by: can <can@anyscale.com>
@can-anyscale can-anyscale requested a review from aslonnie June 28, 2023 02:00
@richardliaw richardliaw merged commit 290bd2d into master Jun 28, 2023
@richardliaw richardliaw deleted the can-validate-wheel branch June 28, 2023 16:58
arvind-chandra pushed a commit to lmco/ray that referenced this pull request Aug 31, 2023
Signed-off-by: can <can@anyscale.com>
Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
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.

3 participants