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

⬆️ Upgrade Starlette to 0.17.1 #4145

Merged
merged 5 commits into from Jan 7, 2022

Conversation

simondale00
Copy link
Contributor

⬆️ Upgrade Starlette to 0.17.0

0.17.0 addresses issue Starlette issue #1255, preventing BaseHTTPMiddleware from raising anyio.ExceptionGroup.

@nkamali
Copy link

nkamali commented Nov 4, 2021

Looks good to me and I'm looking forward to this upgrade for some time. Thank you, @simondale00 for helping with pushing this one forward.

Copy link

@nkamali nkamali left a comment

Choose a reason for hiding this comment

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

lgtm! 👍

@simondale00
Copy link
Contributor Author

Hey @tiangolo! Curious what additional steps I'll need to take to help get this merged. Thanks in advance!

@Kludex
Copy link
Sponsor Collaborator

Kludex commented Nov 8, 2021

Just wait.

@kevr
Copy link

kevr commented Nov 9, 2021

Just wait.

Wait for what?

It's a bit hard to decide what to do at this point.

  1. An MR is up. Wait for the MR to be merged and deal with manual edits to debug things until then.
  2. Depend on a fork of our own or a third party pinned commit.

If the MR is gonna take ten months, then we might as well fork and depend on that. If not, then we would just wait for the new release.

Can we get some type of... idea about the state of this and some sort of rough timeline?

archlinux-github pushed a commit to archlinux/aurweb that referenced this pull request Nov 9, 2021
Starlette 0.16.0 has a pretty bad bug in terms of logging which
has been fixed in the 0.17.0 release. That being said, FastAPI has
not yet merged a request at tiangolo/fastapi#4145
which resolves this dependency resolution so we can use the updated
starlette package.

kevr has forked the pull request in question and we are using it
for now in our poetry dependencies to get ahead of the game.

When FastAPI upstream is updated to support 0.17.0, we'll need
to switch this back to using upstream's source.

Signed-off-by: Kevin Morris <kevr@0cost.org>
@Kludex
Copy link
Sponsor Collaborator

Kludex commented Nov 9, 2021

No one can give you an estimation, sorry.

The only one with merge rights is @tiangolo , and by the time he sees this PR, he'll probably just merge it.

If you don't want to fork, you can also pin a version predecessor to 0.69 (I'm on my phone, but I think it was 0.68.2).

@robertlagrant
Copy link

Is it possible to provide a range of versions of Starlette, or is 0.17.0 the absolute minimum?

@simondale00
Copy link
Contributor Author

Is it possible to provide a range of versions of Starlette, or is 0.17.0 the absolute minimum?

0.17.0 is the absolutely minimum to apply this specific fix, preventing Starlette from raising anyio.ExceptionGroup.

Though to your point, provided that Starlette adheres to semver, it might be more appropriate to pin FastAPI to >=0.17.0,<1.0.0

@Kludex
Copy link
Sponsor Collaborator

Kludex commented Nov 9, 2021

@simondale00 semver only kicks in after 1.0. That's basically why is pinned here.

@adriangb proposed to have a pipeline that runs periodically against the latest Starlette version (or master), and would auto-merge the pin/range. Maybe we can do something about this, but it's up to @tiangolo . 😗

@adriangb
Copy link
Contributor

adriangb commented Nov 9, 2021

@adriangb proposed to have a pipeline that runs periodically against the latest Starlette version (or master), and would auto-merge the pin/range

Auto-bumping would be cool!

But I was thinking something much more basic: just running the pipeline and watching it for errors.
That way we can fix smaller issues (instead of fixing the accumulated issues since the last bump), and we'd also have the opportunity to make changes in Starlette before they cut a release.

@LarsStegman
Copy link

Version 0.17.1 of Starlette has been released, which fixes a bug in the authentication mechanism. I have run into this bug during development. Please use this version instead of 0.17.0.

@yezz123
Copy link
Contributor

yezz123 commented Nov 17, 2021

I guess there is no problem if everything works smoothly, right now the 0.16.0 do all jobs so waiting for Sebastian to bump the to the last version of starlette 🌟

@LarsStegman
Copy link

@simondale00 Could you update the PR to use 0.17.1?

@Kludex
Copy link
Sponsor Collaborator

Kludex commented Nov 22, 2021

It really doesn't matter. @tiangolo will see the PR, and it will update it himself. Just wait for him to check this. 😗

@narolski
Copy link

@tiangolo I do see these exceptions raised a lot during our performance tests. Would you mind taking a look at this?

@kevr
Copy link

kevr commented Dec 1, 2021

This time delay is kind of crazy.

pyproject.toml Outdated Show resolved Hide resolved
@sunshine-joe
Copy link

I also got bug with starlette<0.17.0。
wating for this PR be merged , thanks

@jerber
Copy link

jerber commented Dec 7, 2021

@tiangolo this causes a lot of errors on our server, would be great if you could merge!

@narolski
Copy link

@tiangolo sorry for bugging you again, but only today I have seen this exception raised over 100 times on my pre-prod environments. Is there any way I could help with upgrading the fastapi's dependencies?

@hidaris
Copy link

hidaris commented Dec 17, 2021

@tiangolo sorry for bugging you again, but only today I have seen this exception raised over 100 times on my pre-prod environments. Is there any way I could help with upgrading the fastapi's dependencies?

Maybe you can contact him on twitter, if the problem is really urgent

@kevr
Copy link

kevr commented Dec 17, 2021

Maybe you can contact him on twitter, if the problem is really urgent

It's been urgent for about a month and a half now; check out the backlog of the issue.

@narolski
Copy link

@tiangolo sorry for bugging you again, but only today I have seen this exception raised over 100 times on my pre-prod environments. Is there any way I could help with upgrading the fastapi's dependencies?

Maybe you can contact him on twitter, if the problem is really urgent

I've reached out via DM on Twitter, will let you know here should I get a response. Is there anyone else who can help out in the meantime?

@samuelcolvin samuelcolvin changed the title ⬆️ Upgrade Starlette to 0.17.0 ⬆️ Upgrade Starlette to 0.17.1 Dec 22, 2021
@codecov
Copy link

codecov bot commented Dec 22, 2021

Codecov Report

Merging #4145 (dbb2599) into master (58ab733) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #4145   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          409       409           
  Lines        10264     10270    +6     
=========================================
+ Hits         10264     10270    +6     
Impacted Files Coverage Δ
...test_tutorial/test_dataclasses/test_tutorial002.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58ab733...dbb2599. Read the comment docs.

@github-actions
Copy link
Contributor

📝 Docs preview for commit dbb2599 at: https://61c32593f3060d161428c7ba--fastapi.netlify.app

Copy link
Sponsor Collaborator

@samuelcolvin samuelcolvin 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 tested this on a big project and everything seems to be working great

installed with

pip install git+https://github.com/tiangolo/fastapi.git@refs/pull/4145/merge

@tiangolo LGTM

@jerber
Copy link

jerber commented Jan 5, 2022

@tiangolo could you merge this? Our production app has been getting a ton of errors a day

@dimaqq
Copy link
Contributor

dimaqq commented Jan 5, 2022

Looks like this project needs a backup maintainer

@dimaqq
Copy link
Contributor

dimaqq commented Jan 5, 2022

#4263

@tiangolo tiangolo merged commit 2b10ca1 into tiangolo:master Jan 7, 2022
@tiangolo
Copy link
Owner

tiangolo commented Jan 7, 2022

Thanks @simondale00! 🚀

And thanks a lot @Kludex and @samuelcolvin! Your help is very much appreciated. 🙇 🍰 😎

@simondale00 simondale00 deleted the upgrade-starlette-0.17.0 branch January 7, 2022 12:27
@tiangolo
Copy link
Owner

tiangolo commented Jan 7, 2022

This is now available in FastAPI 0.71.0. 🔖🚀

@samuelcolvin
Copy link
Sponsor Collaborator

Thanks so much @tiangolo.

@alexiri alexiri mentioned this pull request Jan 11, 2022
9 tasks
@Redict
Copy link

Redict commented Jan 15, 2022

@tiangolo when it will be uploaded to pip?

@simondale00
Copy link
Contributor Author

@tiangolo when it will be uploaded to pip?

@Redict It's already available on PyPI: https://pypi.org/project/fastapi/0.71.0/#history

@ashkan-hadadi
Copy link

ashkan-hadadi commented Feb 26, 2022

https://stackoverflow.com/questions/71222144/runtimeerror-no-response-returned-in-fast-api-when-refresh-request

The problem is still alive! please refer to above link @tiangolo

@alex-pobeditel-2004
Copy link

Hello @ashkan-hadadi!
Is this issue still bothering you?
Right now I'm stuck on FastAPI 0.75 (released on March, 5) and issue persists there. But it's interesting for me if it was solved in next versions? Looks like FastAPI got many Starlette updates recently.

@jkinkead
Copy link

@alex-pobeditel-2004 - I'm still seeing the behavior as described in this bug on the latest version, so I think it's still an open issue.

@kozhushman
Copy link

Im using fastapi = 0.78.0 and still facing this issue, sadly

@alex-pobeditel-2004
Copy link

Looks like another fix for it will arrive with next Starlette release:
encode/starlette#1634

@adam-flyr
Copy link

I'm having the same issue here. Noticed that I started to happen after I changed from axios < 0.22 to 0.22. I was using cancelToken before, and AbortController now.

@teamhide
Copy link

teamhide commented Jun 9, 2022

any update?

@Kludex
Copy link
Sponsor Collaborator

Kludex commented Jun 9, 2022

any update?

? 🤔

@Ki6an
Copy link

Ki6an commented Jul 15, 2022

still facing the issue in fastapi = 0.79.0

@Kludex
Copy link
Sponsor Collaborator

Kludex commented Jul 15, 2022

This issue was solved.

If you have an issue that looks like the same, feel free to report as a discussion on the Starlette repository.

The BaseHTTPMiddleware logic is implemented on Starlette.

@acjh
Copy link

acjh commented Jul 15, 2022

Discussion on the Starlette repository: encode/starlette#1527

JeanArhancet pushed a commit to JeanArhancet/fastapi that referenced this pull request Aug 20, 2022
Co-authored-by: Dima Tisnek <dimaqq@gmail.com>
Co-authored-by: simond <simond@surveymonkey.com>
Co-authored-by: Samuel Colvin <s@muelcolvin.com>
Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet