-
Couldn't load subscription status.
- Fork 380
Add python 3.13 support #2658
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
base: main
Are you sure you want to change the base?
Add python 3.13 support #2658
Conversation
- Add Python 3.13 to Github Actions workflow matrices - Update CIBW_PROJECT_REQUIRES - Add classifier to pyproject.toml - Update ray dependency with python 3.13 constraint - Fix InMemoryCatalog to close connections - Add pytest filter to ignore rest of 3.13 SQLite Resource Warnings
| duckdb = { version = ">=0.5.0,<2.0.0", optional = true } | ||
| ray = { version = ">=2.10.0,<=2.44.0", optional = true } | ||
| ray = [ | ||
| { version = ">=2.10.0,<3.0.0", python = ">=3.10", optional = true }, |
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.
We used to have a wider upper bound of 3.0.0 like this, and the tight upper bound was added as part of: #2071 (comment).
I was wondering if you were about to have a single entry since on a clean dep resolve Poetry usually chooses the highest compatible version which should technically work across all python releases.
ray = [
{ version = ">=2.45.0,<=2.49.0", python = ">=3.10", optional = true }
]
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 guess this would help people who are stuck on older ray releases. But I can remove it for sure.
| filterwarnings = [ | ||
| "error", | ||
| # Python 3.13 sqlite3 module ResourceWarnings for unclosed database connections | ||
| "ignore:unclosed database in <sqlite3.Connection object*:ResourceWarning", |
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.
As you seeing this with Make 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.
thats interesting, we should fix this for the tests
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 did try to fix the warnings by closing the database, and some of that is in the PR, but they started breaking other legitimate tests so I ignored it like other projects mostly have (mentioned in the issue linked above)
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.
we can fix this as a follow up pr
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.
| # Ignore 32 bit architectures | ||
| CIBW_ARCHS: "auto64" | ||
| CIBW_PROJECT_REQUIRES_PYTHON: ">=3.10" | ||
| CIBW_PROJECT_REQUIRES_PYTHON: ">=3.10,<=3.13" |
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.
nit: i dont think we need to add an upperbound limit here
| filterwarnings = [ | ||
| "error", | ||
| # Python 3.13 sqlite3 module ResourceWarnings for unclosed database connections | ||
| "ignore:unclosed database in <sqlite3.Connection object*:ResourceWarning", |
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.
thats interesting, we should fix this for the tests
For sure, I'll wait for the other PRs, and I can pick this up again if those are not merged. |
|
feel free to keep on working on this pr, the other 2 prs have not been updated. i just wanted to tag them to link these together |
Rationale for this change
Closes #2530, closes #1372
Are these changes tested?
Yes
Are there any user-facing changes?
No