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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade FastAPI to v0.70.0 #12836

Merged
merged 3 commits into from
Nov 3, 2021

Conversation

davelopez
Copy link
Contributor

@davelopez davelopez commented Nov 2, 2021

I will try to upgrade the FastAPI dependency to the latest version gradually as in the v0.70.0 release notes

  • First to FastAPI 0.68.2, with no breaking changes, but upgrading all the sub-dependencies.
  • Next to FastAPI 0.69.0, which upgrades Starlette to 0.15.0, with AnyIO support, and a higher chance of having breaking changes.
  • Finally to FastAPI 0.70.0, just upgrading Starlette to the latest version 0.16.0 with additional bug fixes.

Note: I'm going to update the dependencies manually because I can't make poetry work in a reasonable amount of time, so I will appreciate expert eyes on this... 馃槄

Reason

There are some bug fixes in both FastAPI and Starlette and we should try to keep in sync with the latest changes as much as we can at least until the new stack becomes the default.
Also, there is a fix for the previous breaking change in 0.65.2 that was forcing us to use JSON Content-type on all payloads, so after this, hopefully, there should be no need to explicitly set json=True in the API tests requests.

How to test the changes?

  • This is a refactoring of components with existing test coverage.

License

@github-actions github-actions bot added this to the 22.01 milestone Nov 2, 2021
@davelopez davelopez marked this pull request as draft November 2, 2021 13:54
Copy link
Member

@jdavcs jdavcs 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 given up on poetry for now - last time I tried to add a dependency and record how long it takes for it to solve the dependency graph, it took 10 hours before it died. So I just added the dependency manually. We may need to find a better way to do this, eventually, but for now, in my opinion, it's fine. (At least the risk of something conflicting - which we will find and fix sooner rather than later - outweighs the potential derailment of any PR that seeks to update a dependency 馃槃

@davelopez davelopez marked this pull request as ready for review November 3, 2021 14:15
@davelopez
Copy link
Contributor Author

davelopez commented Nov 3, 2021

The failing tests seem unrelated:

  • lib/galaxy_test/api/test_libraries.py::LibrariesApiTestCase::test_nonadmin has been flaky for a while.
  • The converter tests are failing apparently because some tool is failing the linting:
    root/lib/galaxy/datatypes/converters/fasta_to_bowtie_base_index_converter.xml
    Failed linting
    Applying linter general... WARNING
    .. WARNING: Requirement bowtie defines no version
    

So I guess there are no breaking changes after the upgrade 馃帀

@mvdbeek mvdbeek merged commit 1a3f4ae into galaxyproject:dev Nov 3, 2021
@davelopez davelopez deleted the upgrade_fastapi_dependencies branch November 3, 2021 15:18
@davelopez
Copy link
Contributor Author

Ok, I've seen exceptions being logged in the currently failing Converters tests in the new pull requests due to anyio.BrokenResourceError.
Apparently is related to tiangolo/fastapi#4041 which is a Starlette issue that seems to be addressed in Starlette v0.17.0

FastAPI v0.70.0 is pinned to Starlette v0.16.0, so I guess we cannot just update to Starlette v0.17.0 yet. Should we downgrade FastAPI to v0.69.0 then or wait for a new release?

@nsoranzo
Copy link
Member

nsoranzo commented Nov 4, 2021

The failing converter test has just been fixed by #12851 (already merged forward), but we probably still want to avoid the anyio.BrokenResourceError .

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

4 participants