-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[24.0] Close install model session when request ends #18629
[24.0] Close install model session when request ends #18629
Conversation
101b3ef
to
ecd7bbb
Compare
This is probably the more general fix for galaxyproject#18627.
ecd7bbb
to
d83b56b
Compare
Is missing in lib/galaxy/webapps/galaxy/buildapp.py but github does not let me add suggestions into the right spot. |
a126f10
to
20cedf7
Compare
Thanks @mvdbeek. I am testing this with the code snippet from #18627 and so far I am not getting any HTTP 500. I'll let the tool installation CI run overnight just to be sure though. Tomorrow I share the results with you and if it worked we can merge. Alternatively, you may also check it yourself here (the link won't work until the the CI ends). Once the job is finished, search for "GET: error 500: b'Internal Server Error'" within the file. |
It showed up many times on the CI logs :( |
What did you test where and which commit ? |
I tested a126f10 (added |
Do you have the logs somewhere ? The tool install log doesn't help. I'm not seeing anything suspicious in sentry. |
I think I see the problem though, if we have FastAPI routes that don't depend on trans or app we're never applying the session scope logic. |
When the issue pops up it does it in this Sentry issue. The events from the last 24 hours correspond to the last tool install run. Most likely many I cannot get you the gunicorn logs from yesterday because the logs have already rotated (too many entries per second). To get them I have to set up some machinery to dump them somewhere else and trigger the failure again (which I may do). I assume you'd want to see the full wall of text during the whole duration of the tool installation CI run right? |
It's ok, I think I see the problem, see #18629 (comment) |
Yes, in the stacktrace from the Sentry issue I linked you can see the request goes through FastAPI (this line). |
@mvdbeek Trying d20b333 out (link to CI log). As usual I let you know once it finishes. |
It works!! 🚀 🚀 |
Are the failing startup tests relevant? |
No, it's 3.12 which isn't compatible with the pydantic version we install, all 24.0 PRs have that problem. |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
This is probably the more general fix for
#18627.
@kysrpex could you test this without the explicit session management you suggested in #18627 ?
How to test the changes?
(Select all options that apply)
License