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

CI: update python versions #4775

Merged
merged 6 commits into from
May 7, 2023
Merged

Conversation

wisp3rwind
Copy link
Member

@wisp3rwind wisp3rwind commented Apr 30, 2023

Given that Python 3.11 is released, we've not been testing the most recent versions anymore. Thus, (try to) use semver x-ranges in order to avoid needing manual updates of this in the future.

Don't merge yet, I'm not sure that the range specification for -dev versions is actually valid as I used it here.
Unfortunately, semver intentionally doesn't give a way to specify 3.x-dev to always get the latest development version. Thus, the 3.12 specification will still need to be updated manually.

@wisp3rwind wisp3rwind force-pushed the pr_update_ci_python branch 3 times, most recently from e08d244 to edfbb44 Compare April 30, 2023 09:03
@wisp3rwind
Copy link
Member Author

Turns out that our 3.11 CI has been failing on Windows already, but nobody noticed because continue-on-error job steps show up as successful...

@sampsyo
Copy link
Member

sampsyo commented May 2, 2023

Aha; that's cool! I didn't realize 3.x was an option; this makes lots of sense to me.

It's nice but also worrisome that this turned up so many perplexing-looking filesystem-related errors on Windows… it looks like something having to do with shutting down importer threads properly? That will certainly be fun to debug. Maybe we should suppress this failure (as it was being unintentionally suppressed before) in order to merge this PR and work on that separately?

@wisp3rwind wisp3rwind force-pushed the pr_update_ci_python branch 2 times, most recently from 0ca83c8 to 2e8da06 Compare May 4, 2023 06:36
@wisp3rwind
Copy link
Member Author

It's nice but also worrisome that this turned up so many perplexing-looking filesystem-related errors on Windows… it looks like something having to do with shutting down importer threads properly? That will certainly be fun to debug. Maybe we should suppress this failure (as it was being unintentionally suppressed before) in order to merge this PR and work on that separately?

I'll make an attempt to fix it quickly here; if that doesn't work this sounds like a good strategy!

The failures happen mostly in test shutdown (where the database file seems to unexpectedly remain open), but there's a different failure in the replaygain tests (due to an unexpectly missing threads config value), as well as a failure in PlaylistTestRelativeToLib.test_name_query_with_absolute_paths_in_playlist which is (superficially at least) different.
So, at least part of the issue is related to multi-threaded execution.

One thing that I noticed is that we never explicitly close sqlite3 connections (But leave that up to the GC I presume?). Doing so seems to fix the dbcore migration tests. Unfortunately, this is much harder to fix in general: I attempted to close the connections in dbcore.db.Database._close, but this errors out saying that sqlite3 objects can only be used from the threads that created them. Which would mean we need to ensure that any thread that shuts down closes its connection. I expect that to be quite tricky.

Maybe, someone (...) should sift through the sqlite3 source code changes between 3.10 and 3.11 and see whether any detailed explanation and rational can be found?

@wisp3rwind
Copy link
Member Author

We're not the first to discover this: python/cpython#97641

@wisp3rwind
Copy link
Member Author

From the discussion over there, explicitly closing sqlite3 connections seems to be the right fix; and indeed all the tests are green now. I guess the other test failures were a result of incomplete test cleanup due the exception in test teardown.

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Wow; this looks like a tricky issue, according to python/cpython#97641 (comment). Nice work tracking this down. It's a relief that it suffices to close everything in our existing self._connections dict!

beets/dbcore/db.py Outdated Show resolved Hide resolved
(try to) use semver x-ranges in order to avoid needing manual updates of
this in the future
on Python 3.11, the Windows CI started crashing due to the database file
remainig open unexpectly in test shutdown; this attemps to fix that
- some helper functions were incorrectly detected as being tests due to
  their name
- use of the deprecated assertEquals alias
which we rely on. This test doesn't actually work for Python < 3.11,
since Python used to hardcode the threadsafety value to 1
@wisp3rwind
Copy link
Member Author

Wow; this looks like a tricky issue, according to python/cpython#97641 (comment). Nice work tracking this down.

Well, luckily, I didn't need to track down the Cpython side of this. I could have saved quite some time by immediately searching for this problem in their issue tracker 😆

It's a relief that it suffices to close everything in our existing self._connections dict!

Luckily, the check_same_thread flag exists, so that doing this is in fact viable. Initially, I didn't know about it; and at that point things would indeed have become very tricky

@wisp3rwind wisp3rwind merged commit 8fc3dde into beetbox:master May 7, 2023
@wisp3rwind wisp3rwind deleted the pr_update_ci_python branch May 7, 2023 17:22
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.

3 participants