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

[release] Handle exceptions on requests failure when retrieving wheel #28685

Merged
merged 4 commits into from Oct 29, 2022
Merged

[release] Handle exceptions on requests failure when retrieving wheel #28685

merged 4 commits into from Oct 29, 2022

Conversation

rickyyx
Copy link
Contributor

@rickyyx rickyyx commented Sep 22, 2022

Signed-off-by: rickyyx rickyx@anyscale.com

Why are these changes needed?

Exceptions currently not caught when getting wheel URL, causing tests to fail

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 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 :(

@rickyyx
handle exceptions on requests
d62c827 
Signed-off-by: rickyyx <rickyx@anyscale.com>
@rickyyx
Copy link
Contributor Author

rickyyx commented Sep 22, 2022

Any thoughts on testing? I could trigger a release tests to make sure it doesn't break anything

simon-mo
simon-mo approved these changes Sep 22, 2022
View reviewed changes
release/ray_release/util.py Outdated Show resolved Hide resolved
@simon-mo
Copy link
Contributor

simon-mo commented Sep 22, 2022 •
edited

Any thoughts on testing?

Good thinking, alternatively you can try trigger a release test pointing to a non-existent host like wheel_url = "localhost:1234..." which will for sure throw

@rickyyx
nit
62b680b 
Signed-off-by: rickyyx <rickyx@anyscale.com>
krfricke
krfricke reviewed Sep 23, 2022
View reviewed changes
Copy link
Contributor

@krfricke krfricke left a comment

Choose a reason for hiding this comment

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

For testing, maybe just add a unit test and try to resolve an invalid URL?

release/ray_release/util.py Show resolved Hide resolved
@krfricke krfricke added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Oct 4, 2022
Kai Fricke added 2 commits October 28, 2022 17:00
Add unit test
9aa0ffc 
Signed-off-by: Kai Fricke <kai@anyscale.com>
Merge branch 'master' into ci/release/url-exists
3a8214b
krfricke
krfricke approved these changes Oct 29, 2022
View reviewed changes
Copy link
Contributor

@krfricke krfricke left a comment

Choose a reason for hiding this comment

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

I've added a unit test and will merge after CI pass

@krfricke krfricke removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Oct 29, 2022
@krfricke krfricke merged commit 399d383 into ray-project:master Oct 29, 2022
WeichenXu123 pushed a commit to WeichenXu123/ray that referenced this pull request Dec 19, 2022
@rickyyx @WeichenXu123
[release] Handle exceptions on requests failure when retrieving wheel (…
763b51d 
…ray-project#28685)

Exceptions currently not caught when getting wheel URL, causing tests to fail

Signed-off-by: rickyyx <rickyx@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Co-authored-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Weichen Xu <weichen.xu@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krfricke krfricke krfricke approved these changes

@simon-mo simon-mo simon-mo approved these changes

@rkooo567 rkooo567 Awaiting requested review from rkooo567

Assignees

@krfricke krfricke

@simon-mo simon-mo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rickyyx @simon-mo @krfricke