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

Add python 3.13, Remove python 3.8 #345

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

sneakers-the-rat
Copy link
Contributor

@sneakers-the-rat sneakers-the-rat commented Oct 9, 2024

Sibling of: linkml/linkml#2358
Fix: linkml/linkml#2370
Fix: linkml/linkml#2378

does what it says on the tin.

Currently linkml/linkml#2358 is failing because there's a from typing import re statement in here, so i also removed that.

I added the upgrade ruff rules and one to check for unused imports just to automate the update, and i figure why the heck not leave them in here i would actually really like to at least isort stuff in here if not give it the same linting rules as upstream.

Other stuff

  • Removed the monkeypatch to allow arbitrary kwargs in dataclasses - if we want to keep this in then someone else re-implement it, but the basis for doing that was removed in python 3.13 and it seems like the thing it was supposed to do (report line numbers from instantiation errors) is better done at yaml loading time rather than object instantiation time to me, since the only time it would work anyway is after yaml has been loaded with the loader that would be able to report the error in the first place.
  • The tox files have never worked for me with poetry, and it doesn't look like they've been used in a long time anyway (still using whitelist_externals rather than allowlist_externals ) so i fixed them to just work with normal standards-compliant installation rather than wrestle with accursed poetry, but i can take that out if it's not wanted.
  • added an __all__ to linkml_runtime.linkml_model because importing '*' from it makes linters complain.
  • fix pydantic deprecation warnings like dict -> model_dump and parse_obj -> model_validate, which i swear i have done like 20 times but it keeps showing up somehow lol
  • add a __test__ = False flag to TestEnvironment so pytest doesn't try to treat it like a test.

@vincentkelleher
Copy link

Just ran the tests on my local environment and things look fine 👍

image

Maybe you could add "Programming Language :: Python :: 3.13" to the pyproject.toml file ?

image

@sneakers-the-rat
Copy link
Contributor Author

sneakers-the-rat commented Jan 24, 2025

Done.

Can this get merged? Would be nice to support python 3.13 and not need to keep rebasing/deconflicting this and the upstream PR

@vincentkelleher
Copy link

@sneakers-the-rat I've approved this PR but I don't have the rights to merge it.

@ptgolden
Copy link

@amc-corey-cox ran into this recently when trying to run schema automator under Python 3.13

@sneakers-the-rat I almost made an issue pretty similar to linkml/linkml#2370, but noticed you'd already done the lifting!- Thanks

@sierra-moxon
Copy link
Member

@sneakers-the-rat @ptgolden - can you please fix the failing tests and then carry this through the finish line?

@ptgolden
Copy link

The test failures are because upstream linkml still advertises support for python 3.8. It seems that those failures will continue until linkml/linkml#2358 is merged.

@sierra-moxon
Copy link
Member

Unfortunately, I don't think we want to merge the upstream one until it has passing tests or at least a comment on why they can't/won't be passable. But I trust @sneakers-the-rat is already on fixing those and we'll have this out in no time. Alternatively, if you have some cycles @ptgolden, and you want to fix the forked PR upstream, that would be wonderful.

@ptgolden
Copy link

This should be the first PR that should be merged out of the two. When a new version is tagged here for a release that supports 3.13, that new version can be pinned in the linkml PR for updating to 3.13.

I think the fix here is to change the test_upstream action so that we can install the local version of linkml-runtime even if an incompatible python version is declared. That might mean changing these steps:

# we are not using linkml-runtime's lockfile, but simulating what will happen
# when we merge this and update linkml's lockfile
- name: add linkml-runtime to lockfile
working-directory: linkml
run: poetry add ../linkml-runtime
# note that we run the installation step always, even if we restore a venv,
# the cache will restore the old version of linkml-runtime, but the lockfile
# will only store the directory dependency (and thus will reinstall it)
# the cache will still speedup the rest of the installation
- name: install linkml
working-directory: linkml
run: poetry install --no-interaction -E tests

To run something like:

# in linkml directory
poetry install --no-interaction -E tests
poetry run pip install ../linkml-runtime

This would mean that there would be a Poetry lockfile that doesn't represent the state of the virtual environment, but it would allow installing the local version of linkml-runtime when poetry add would reject it (because of incompatible python versions).

I'd push a commit here but I'll wait to hear thoughts from @sneakers-the-rat

@ptgolden
Copy link

ptgolden commented Jan 31, 2025

To be clear, the dilemma here was caused by two things (both totally understandable on their own):

  1. A reverse dependency on the state of upstream linkml here because of the test_upstream action
  2. Dropping support for 3.8 at the same time as adding support for 3.13 (and in the corresponding linkml PR)

Because upstream linkml currently requests 3.8 to 3.12, Poetry can not add this branch as dependency because it doesn't support 3.8.

Upstream linkml can't slide the window from 3.8 to 3.12 -> 3.9 to 3.13 because the current linkml-runtime release does not support 3.13.

(that cycles back and forth forever)

In retrospect, the order could have been:

  1. Support 3.13 in linkml-runtime
  2. Support 3.13 and drop support for 3.8 in linkml
  3. Drop support for 3.8 in linkml-runtime

The solution I proposed above is just a kludge to break out of the cyclical trap these two PRs are in, while still being able to run the test_upstream action.

@sierra-moxon
Copy link
Member

from dev call:

  • check in with @sneakers-the-rat for feedback
  • would like to consider closing this linkml-runtime PR in favor of a smaller one that just expands support for 3.13, then merging the linkml PR etc... following @ptgolden's suggestion above:
Support 3.13 in linkml-runtime
Support 3.13 and drop support for 3.8 in linkml
Drop support for 3.8 in linkml-runtime

@sneakers-the-rat
Copy link
Contributor Author

I mean it's really not a big deal we just need to merge at the same time, but sure if yall want to redo this be my guest I guess

@sneakers-the-rat
Copy link
Contributor Author

in linkml directory

poetry install --no-interaction -E tests
poetry run pip install ../linkml-runtime

This is how it used to work, but the current version also testing compatibility of dependencies is a feature not a bug. It could be two steps, checking for dep compatibility and running tests, but it's like that for a reason

@dalito
Copy link
Member

dalito commented Feb 1, 2025

@sierra-moxon Maybe important: who is expected to merge?

I could do it from the permissions but don't really know where to use the power without making you upset. See also https://github.com/orgs/linkml/discussions/2518

@sneakers-the-rat
Copy link
Contributor Author

Support 3.13 in linkml-runtime
Support 3.13 and drop support for 3.8 in linkml
Drop support for 3.8 in linkml-runtime

this is not really possible, since the major thing these PRs do is update the code to remove 3.8 features (typing.Dict et al), so if we really want to see green checks we could fake it and leave the linkml-runtime lower bound at 3.8 (while still not testing at 3.8 because those tests would fail), merge both PRs, and then follow up with a trivial PR that just boosts the floor. alternatively we could do this in 4 PRs where we support 3.8-3.13 in both, then remove 3.8 in both, but imo it's simpler to just "merge the work that has already been done at the same time" than "make 4 new PRs"

@dalito
Copy link
Member

dalito commented Feb 1, 2025

You could remove 3.8 first and then add 3.13 (so the opposite order). Maybe next time for 3.9 / 3.14 😉

@sneakers-the-rat
Copy link
Contributor Author

it would be the same problem, bumping the bottom means the linkml-runtime upstream tests will fail for the version inconsistency problem you experienced yesterday, then later when we bump the top, if there are breaking changes in python (like the typing.io change) linkml will fail because linkml-runtime will be out of date. in this pair of PRs we happened to get both of those problems at the same time, but again there really isn't a problem at all - the solution is just to merge both at the same time (or, perhaps, merge linkml-runtime first after having done a local test against upstream (which is what i'm doing now), then update the lockfile with the new version to trigger tests and merge here after that is released on pypy)

@sierra-moxon
Copy link
Member

sierra-moxon commented Feb 1, 2025

@sneakers-the-rat I do like to see green checks, yes. @dalito - I would hold off on merging this just yet. We did discuss a few different paths at the dev call, one of which was to ignore the red x's this time around. For now, I would like to try the approach above, in a separate, atomic PR and be happily wrong if it fails too (and we can put in some tix to try and reconcile this process for next time).

@sneakers-the-rat
Copy link
Contributor Author

sneakers-the-rat commented Feb 1, 2025

hit me with an approval to trigger the upstream tests. or idk trigger them manually, i can't do it because i don't have commit privs for this repo.

edit: working through this now. another source of error is that the means of doing the dataclass init monkeypatch here was removed in python 3.13, so we need to do sync'd PRs regardless - if we want to do this in smaller PRs that all have green checks, we're now up to 4

  • drop the dataclass init monkeypatch in Pythongen in linkml
  • support python 3.13 in linkml-runtime
  • support python 3.13 in linkml, drop 3.8
  • drop 3.8 from linkml-runtime

I'm frankly a little annoyed. I am at no joke 20+ hours on this pair of PRs, dealing with and cleaning up a bunch of tech debt along the way that i didn't create, all of which is relevant to the PR at hand (adding ruff rules to check for out of date code also reveals other linter errors). that 20 hours includes what looks like will be another 4 hours today rebasing and fixing problems caused by not having merged these since october. but now we want to just drop this and start again?

@ptgolden
Copy link

ptgolden commented Feb 1, 2025

Hey @sneakers-the-rat, I understand your frustration and I wasn't trying to create more work for you. I really appreciate all the work you've done here and the paired PR. I stumbled upon the problem because I came across the monkeypatch of dataclasses and had a similar reaction as you did.

It looks like you've pushed a commit to fake 3.8 compat, which I think is the best thing to do to save a bunch of time splitting this up into multiple chunks while satiating the automated checks. (To be clear, I would have been happy to have taken up that cherry picking/rebasing, but I didn't want to do so without checking with you).

This was an exceptional situation where both libraries were raising the floor and bumping the ceiling of Python at the same time, but I imagine this situation has/does/will pop up when doing the same process with dependencies. If I'm not mistaken, it's what you're talking about here: #361 (comment)

This seems to be an issue that's inherent to updating dependencies for tightly-coupled-but-separate libraries in different repos while also being strict about automated testing between them. The choices seem to be either the 3/4 step dance we've been discussing, or synced PRs and overlooking tests that are expected to fail because of incompatibilities. The former sounds nice and will result in nice green checks, but, depending on what's being updated, could result in a lot of unnecessary nasty work of the sort that we remember from the python 2->3 upgrade.

@sneakers-the-rat
Copy link
Contributor Author

Hang on I think I can fix this by allowing one to specify an upstream branch to test against, one sec

@sneakers-the-rat
Copy link
Contributor Author

sneakers-the-rat commented Feb 1, 2025

【"here we go"】

1956176_6_articleinline_Super_20Mario_2064

sneakers-the-rat#1
#367

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants