-
Notifications
You must be signed in to change notification settings - Fork 81
make terracotta compatible with python 3.13 #364
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
Merged
Merged
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
647b4a7
make terracotta compatible with python 3.13
jkittner 5808695
pin marshmallow to <4
dionhaefner 9cbb4dd
make terracotta compatible with python 3.13
jkittner 360204f
Merge branch 'python3-13' of github.com:jkittner/terracotta into pyth…
dionhaefner f637162
ignore sqlite3 resource warning
dionhaefner 2503aa4
roll back marshmallow pin
dionhaefner 622aeeb
also upgrade conda tests
dionhaefner File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you help me out on this? It passed with 3.12 the way it was, but for 3.13 this change was needed. But I believe this now ignores more than before, which is not a good thing...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was warning being raised?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what we are getting, which should be ignored by this (or probably cleaned up) but so far everything looks good - all
connectseem to use a context manager...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, that's really weird.
Why would an error like this be unraisable...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless we have a good idea as to why this happens, let's ignore it selectively – that is, something like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue lies deeper, I believe. I also get this for sqlite, but only sometimes. So this could be many things. The
connectmethod not closing properly or the engine not being disposed properly? I tried defining__del__for the class disposing the engine, that reduced the number of warnings, but still did not fix all of them.that's why we unfortunately would have to keep the more generic ignore...
Not sure how to solve that - maybe using sessions might fix that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the warnings you're seeing all the same or are some different to what you posted above?
(I can't reproduce this locally rn since I don't have a working MySQL install)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just ran the dbs in docker compose:
The full output I get is this:
errors
This is very inconsistent and it's not always the same (number of) tests that fail or throw the warning. If you only run parts of the test suite, the errors/warnings change or become more frequent. If you add test randomization e.g. using
randomlyeverything breaks so in addition, there is some test pollution going on as well...Could resource warnings also be some multi-processing/race condition issue?
so IMO the only way forward to at least make it work for 3.13 is ignoring these warnings for now and in the long term try to find the test pollution that are likely caused by the database state not being reset for each test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I can reproduce it now. No luck with isolating this issue though, so I opted to ignore that specific warning, which also seems to be what other projects do. I suspect this is an issue somewhere within the bowels of SQLAlchemy, since we at no point interact directly with
sqlite3.Not closing a SQLite database should be pretty benign to begin with so I think we're okay for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's good to see that others have the same issue. I am fine with ignoring this for now and will subscribe to the other issue - maybe it can be resolved one way or another 👍