-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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/release] Update to recent anyscale API changes #21149
Conversation
Hey @krfricke do you know why it wasn't communicated to us? This is really bad some APIs have deprecated without us noticing it (and didn't have enough migration time). |
I think there is no process in place to track this. Product is not aware of the APIs we're using. I think a solution will be, once we split up e2e.py, to run some unit tests on the product CI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will work, but get_auth_api_client
is not actually a public API either. Only imports under anyscale.sdk
are considered public, so it would be good to eventually move to using those instead. It looks like session_controller
is currently being used to call deprecated CLI commands (push
, pull
), so we should either add more functionality in the SDK or restructure the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Kai!
@alanwguo FYI |
@rkooo567, agreed. This shouldn't have happened like this.
The anyscale CLI/SDK codebase is closed source. Because of this, we think it's reasonable to expect that only the things that are disclosed in our documentation are considered public apis. Hopefully this won't be a problem in the future once the e2e tests are fully using public APIs. |
I see. This makes sense! @krfricke do we have other private api use cases? Maybe just removing all of them asap would help us in the long term. |
Merged master, test run here: https://buildkite.com/ray-project/periodic-ci/builds/2225 |
Why are these changes needed?
Recent changes in the anyscale API rendered the current e2e script incompatible. This PR resolves these subtle API changes.
Related issue number
Checks
scripts/format.sh
to lint the changes in this PR.