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

[Serve] Pin the fastapi & starlette version to avoid breaking proxy #42740

Merged
merged 6 commits into from
Jan 29, 2024

Conversation

edoakes
Copy link
Contributor

@edoakes edoakes commented Jan 26, 2024

Why are these changes needed?

Cherry-pick #42682 and #42747 and #42754.

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

…ay-project#42682)

Pin the fastapi version to 0.108.0. The latest version of fastapi is breaking with starlette. (issue)
Add starlette compact for supporting >= 0.35.0 version.

---------

Signed-off-by: Sihan Wang <sihanwang41@gmail.com>
Co-authored-by: Edward Oakes <ed.nmi.oakes@gmail.com>
@architkulkarni
Copy link
Contributor

@edoakes is this failure relevant?


# Test FastAPI
--
  | serve.run(MyFastAPIDeployment.bind(), name="FastAPI")
  | >       assert requests.get("http://127.0.0.1:8000/hello").text == '"Hello, world!"'
  | E       assert 'got f' == '"Hello, world!"'
  | E         - "Hello, world!"
  | E         + got f
  |  
  | python/ray/serve/tests/test_api.py:432: AssertionError


https://buildkite.com/ray-project/premerge/builds/17576#018d469c-5fba-4657-b086-80e4b927a6c6/6-4438

@edoakes
Copy link
Contributor Author

edoakes commented Jan 26, 2024

@architkulkarni I believe that's a flake, re-running

@edoakes
Copy link
Contributor Author

edoakes commented Jan 26, 2024

Hmm looks like it's consistently failing. Let me take a closer look.

@aslonnie
Copy link
Collaborator

@edoakes , you can sync/rebase on releases/2.9.2, and will have the other core/data tests fixed.

@edoakes
Copy link
Contributor Author

edoakes commented Jan 26, 2024

This one has me a bit stumped. Not able to repro the failure with the same pinned dependencies locally. Might take some time to resolve.

`<= version.parse("0.34.0")` is incorrect because `"0.34.1"` or similar would evaluate to `False`.

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
@edoakes
Copy link
Contributor Author

edoakes commented Jan 26, 2024

Ok, looks like there was some kind of behavior change in redirect handling either in fastapi or starlette.

Working version:

INFO 2024-01-26 15:40:49,444 proxy 127.0.0.1 831aae9b-614b-4a41-b7cd-5922a3dc762a /hello proxy.py:480 - GET 307 16.8ms
INFO 2024-01-26 15:40:49,453 proxy 127.0.0.1 275fdd40-36c3-4873-ab86-fd91fbfe2b84 /hello/ proxy.py:480 - GET 200 7.8ms

Broken version:

INFO 2024-01-26 20:12:28,852 proxy 172.16.0.4 7fefe3cd-fa67-46ce-84fc-ff08ea8b5763 /hello proxy.py:480 - GET 307 20.3ms
INFO 2024-01-26 20:12:28,858 proxy 172.16.0.4 b233ec00-9a66-4978-9015-7d4b8610f2b5 / proxy.py:480 - GET 200 5.5ms

Seems this has uncovered some additional bug. Let me try dropping the versions down.

@edoakes
Copy link
Contributor Author

edoakes commented Jan 26, 2024

Ok, I ran the test manually after installing in a fresh conda env from the built wheel and it passed, so I think it's actually a testing setup issue. Looks like the python/requirements.txt file was not updated in tandem with setup.py. This might be the issue.

Pushing a commit to verify, if it works I'll make a PR to master and then cherry-pick here.

@edoakes
Copy link
Contributor Author

edoakes commented Jan 26, 2024

That was not the correct fix (but it's also needed).

It turns out that the minimal build docker image is actually pulling dependency requirements from the latest release version of Ray rather than the one built in CI:

Logs from build:

#9 30.37 Collecting ray[serve]
--
  | #9 30.40   Downloading ray-2.9.1-cp38-cp38-manylinux2014_x86_64.whl.metadata (13 kB)
  | #9 30.53 Collecting click>=7.0 (from ray[serve])
  | #9 30.54   Using cached click-8.1.7-py3-none-any.whl.metadata (3.0 kB)
  | #9 30.57 Collecting filelock (from ray[serve])
  | #9 30.57   Using cached filelock-3.13.1-py3-none-any.whl.metadata (2.8 kB)
  | #9 30.61 Collecting jsonschema (from ray[serve])
  | #9 30.61   Using cached jsonschema-4.21.1-py3-none-any.whl.metadata (7.8 kB)
  | #9 30.66 Collecting msgpack<2.0.0,>=1.0.0 (from ray[serve])
  | #9 30.67   Using cached msgpack-1.0.7-cp38-cp38-manylinux_2_17_x86_64.manylinux2014_x86_64.whl.metadata (9.1 kB)
  | #9 30.67 Requirement already satisfied: packaging in /opt/miniconda/lib/python3.8/site-packages (from ray[serve]) (23.2)
  | #9 30.85 Collecting protobuf!=3.19.5,>=3.15.3 (from ray[serve])
  | #9 30.85   Using cached protobuf-4.25.2-cp37-abi3-manylinux2014_x86_64.whl.metadata (541 bytes)
  | #9 30.89 Collecting pyyaml (from ray[serve])
  | #9 30.89   Using cached PyYAML-6.0.1-cp38-cp38-manylinux_2_17_x86_64.manylinux2014_x86_64.whl.metadata (2.1 kB)
  | #9 30.91 Collecting aiosignal (from ray[serve])
  | #9 30.91   Using cached aiosignal-1.3.1-py3-none-any.whl (7.6 kB)
  | #9 30.96 Collecting frozenlist (from ray[serve])
  | #9 30.96   Using cached frozenlist-1.4.1-cp38-cp38-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_17_x86_64.manylinux2014_x86_64.whl.metadata (12 kB)
  | #9 30.96 Requirement already satisfied: requests in /opt/miniconda/lib/python3.8/site-packages (from ray[serve]) (2.28.1)
  | #9 31.03 Collecting fastapi (from ray[serve])
  | #9 31.04   Downloading fastapi-0.109.0-py3-none-any.whl.metadata (24 kB)

ci/docker/min.build.Dockerfile Outdated Show resolved Hide resolved
@edoakes edoakes mentioned this pull request Jan 27, 2024
Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
@edoakes edoakes force-pushed the cp-pin-pydantic branch 2 times, most recently from 4f04677 to e4a7383 Compare January 27, 2024 14:28
Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
@edoakes
Copy link
Contributor Author

edoakes commented Jan 27, 2024

Merged in the changes from #42766 to verify that the test passes

@architkulkarni architkulkarni merged commit 5709326 into ray-project:releases/2.9.2 Jan 29, 2024
17 checks passed
@edoakes edoakes self-assigned this Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants