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

[dashboard] restrict aiohttp version #46902

Merged
merged 3 commits into from
Aug 1, 2024

Conversation

ruisearch42
Copy link
Contributor

@ruisearch42 ruisearch42 commented Jul 31, 2024

Why are these changes needed?

aiohttp's new 3.10.0 stops supporting creating ClientSession in non async context any more.

aio-libs/aiohttp#8555 (comment)

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

@ruisearch42 ruisearch42 requested a review from a team as a code owner July 31, 2024 19:56
@@ -48,7 +48,7 @@ scikit-image==0.21.0
prometheus_client>=0.7.1
pandas
tensorboardX
aiohttp>=3.7,!=3.9.0
aiohttp>=3.7,<3.9
Copy link
Collaborator

Choose a reason for hiding this comment

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

w00t, we need to make it possible to go above 3.9 so that ray can be installed with python 3.12; this change has been here when the tests pass previously too i think

Copy link
Collaborator

Choose a reason for hiding this comment

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

based on the slack thread, perhaps change this to <3.10.0, and then you don't need other changes to the requirements file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me make the change and test it.

Signed-off-by: Rui Qiao <[email protected]>
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.

@ruisearch42 ruisearch42 added the go add ONLY when ready to merge, run all tests label Jul 31, 2024
@aslonnie aslonnie requested review from jjyao and edoakes July 31, 2024 23:31
@aslonnie
Copy link
Collaborator

adding @jjyao and @edoakes for the aiohttp<3.10.0 bit.

cc @alanwguo too.

I don't think we have any other choice short-term wise..

@@ -48,7 +48,7 @@ scikit-image==0.21.0
prometheus_client>=0.7.1
pandas
tensorboardX
aiohttp>=3.7,!=3.9.0
aiohttp>=3.7,<3.10.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

you might still want to keep the !=3.9.0 at the same time?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am assuming the !=3.9.0 was there for some reason..

Copy link
Collaborator

Choose a reason for hiding this comment

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

you also need to update the setup.py

Copy link
Collaborator

@can-anyscale can-anyscale Jul 31, 2024

Choose a reason for hiding this comment

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

it used to be <3.9.0 and I changed it to !=3.9.0; nothing specific about 3.9.0, <3.10.0 is properly fine (this file is only used to build the ray docker image, not user-impacting)

Signed-off-by: Rui Qiao <[email protected]>
@aslonnie aslonnie changed the title deflake aiohttp [dashboard] restrict aiohttp version Jul 31, 2024
@aslonnie
Copy link
Collaborator

@alanwguo should we create an issue and assign to you for all the ClientSession (and alike) usage?

@alanwguo
Copy link
Contributor

@alanwguo should we create an issue and assign to you for all the ClientSession (and alike) usage?

I can take the dashboard related ones. Looks like there's a bunch of usage in ray serve as well. @akshay-anyscale's team should own that.

@edoakes
Copy link
Contributor

edoakes commented Aug 1, 2024

@alanwguo should we create an issue and assign to you for all the ClientSession (and alike) usage?

I can take the dashboard related ones. Looks like there's a bunch of usage in ray serve as well. @akshay-anyscale's team should own that.

The serve ones are trivial and only used in tests, I'll put up a PR shortly

Copy link
Contributor

@edoakes edoakes left a comment

Choose a reason for hiding this comment

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

Let's make sure this pin is very temporary

@can-anyscale can-anyscale enabled auto-merge (squash) August 1, 2024 16:50
edoakes added a commit that referenced this pull request Aug 1, 2024
…ext (#46917)

<!-- Thank you for your contribution! Please review
https:/ray-project/ray/blob/master/CONTRIBUTING.rst before
opening a pull request. -->

<!-- Please add a reviewer to the assignee section when you create a PR.
If you don't have the access to it, we will shortly find a reviewer and
assign them to your PR. -->

## Why are these changes needed?

See: #46902

## Related issue number

<!-- For example: "Closes #1234" -->

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

Signed-off-by: Edward Oakes <[email protected]>
@can-anyscale
Copy link
Collaborator

still failing, this is a cache-poisoning issue, @ruisearch42 can you remove this line here https:/ray-project/ray/blob/master/ci/docker/base.test.Dockerfile#L56 in this PR just to invalidate the cache, thankks

Signed-off-by: Rui Qiao <[email protected]>
@github-actions github-actions bot disabled auto-merge August 1, 2024 18:08
@can-anyscale can-anyscale merged commit 820d953 into ray-project:master Aug 1, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants