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 support for Python 3.12 #1704

Merged
merged 15 commits into from
Mar 26, 2024
Merged

Add support for Python 3.12 #1704

merged 15 commits into from
Mar 26, 2024

Conversation

Silvanoc
Copy link
Contributor

@Silvanoc Silvanoc commented Nov 9, 2023

Add test to validate that "greenlet" is on a version that makes the project runnable on Python v3.12.

Update the Poetry locking file to install such a version. Should someone try to downgrade "greenlet" to a version that cannot run on Python 3.12, the test will fail.

Copy link

codecov bot commented Nov 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.68%. Comparing base (aa75b4b) to head (51e3e3e).

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #1704    +/-   ##
========================================
  Coverage   80.68%   80.68%            
========================================
  Files         104      104            
  Lines       11624    11624            
  Branches     2909     3330   +421     
========================================
  Hits         9379     9379            
  Misses       1701     1701            
  Partials      544      544            

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cmungall
Copy link
Member

It looks like older versions of prefixmaps over-pinned the version of greenlet:

So perhaps bumping the minimum prefixmaps might get around the issues here?

In general prefixmaps should be light and unopinionated, see also:
linkml/prefixmaps#54

@Silvanoc
Copy link
Contributor Author

So perhaps bumping the minimum prefixmaps might get around the issues here?

That's what I originally thought and why I started working on it. Unfortunately it is much more complex than that. Please have a look at this PR: linkml/prefixmaps#53.

Initially it looked easy, until I've realized that CI caching was resulting in the object under test (package dependency resolution) not being tested. Once I've introduced python version specific caches, further issues has appeared. Unfortunately right now I don't have time to work on it.

@Silvanoc
Copy link
Contributor Author

Thanks to @cmungall PR linkml/prefixmaps#54 I've been able to resolve the issues I had on PR linkml/prefixmaps#53. Merging my PR, releasing a new prefixmaps version with the fix and upgrading prefixmaps to that version here might help easily fix it here too.

@cmungall
Copy link
Member

looks like still getting that pesky greenlet error...

@Silvanoc
Copy link
Contributor Author

looks like still getting that pesky greenlet error...

I haven't been following this topic for some time. I'll give make sure to rebase all the PRs I've done WRT Python 3.12 and then update the status accordingly on the comments.

@Silvanoc
Copy link
Contributor Author

Having support for Python 3.12 in this project doesn't bring anything until linkml-runtime also supports it. I've had some issues to get it fixed there, but it now looks as if a working PR would be ready. I'll try to prepare here also the required changes (for example, removing support for Python <3.9) for the moment when linkml-runtime supports it.

@Silvanoc
Copy link
Contributor Author

Silvanoc commented Feb 17, 2024

I'm having difficulties with the installation of numpy on Python 3.12 due to an incompatibility of a higher numpy version with Python 3.8. I suposse that the use of Poetry restricted dependencies would help resolving the dilemma, but I lack the expertise to get it done. @pkalita-lbl you seem to be quite fit in Python, could you please have a look at it? Thanks!

Fixed using Multiple constraints dependencies.

@dalito
Copy link
Collaborator

dalito commented Feb 17, 2024

The solution (I think) is just a little further down under Multiple constraints dependencies.

@Silvanoc
Copy link
Contributor Author

The solution (I think) is just a little further down under Multiple constraints dependencies.

I thought the same right after my post, that's what I'm trying right now :-) Thanks for the hint anyway.

pyproject.toml Show resolved Hide resolved
@Silvanoc
Copy link
Contributor Author

On Python 3.12 following test is failing: test_data_types with input ("date", "20210101", False).

But I fail to understand why the test has started failing on Python 3.12 and not on previous Python versions. Python 3.12 is removing lots of long deprecated functions, what could be the source of the failure.

@dalito
Copy link
Collaborator

dalito commented Feb 19, 2024

Since Python 3.11, date.fromisoformat(...) also accepts YYYYMMDD format, see https://docs.python.org/3/library/datetime.html#datetime.date.fromisoformat - so testing with a date in such format is no longer giving the expected failure. Since no tests are configured for 3.11, it only looks like a 3.12 issue.

@Silvanoc
Copy link
Contributor Author

It then means that the failing test: test_data_types should accept 20210101 as a valid date and therefore get input ("date", "20210101", True)... Let's try it out.

Silvanoc added a commit that referenced this pull request Feb 19, 2024
Since Python 3.11 date format "YYYYMMDD" is valid [1]. Since there where no
tests on 3.11, it hasn't been catched until 3.12.

See here [2] for more detail.

[1]: https://docs.python.org/3/library/datetime.html#datetime.date.fromisoformat
[2]: #1704 (comment)

Signed-off-by: Silvano Cirujano Cuesta <silvano.cirujano-cuesta@siemens.com>
@Silvanoc
Copy link
Contributor Author

@dalito I don't think that this test should be testing which formats are accepted by Python, but rather that only those dates that are valid xsd:date are accepted. And my proposed solution would be doing exactly the opposite: making sure that CCYYMMDD is accepted, though it's not a valid xsd:date.

It means that LinkML cannot rely on python datetime library to reject CCYYMMDD dates and will require some own business logic for date validation.

@Silvanoc
Copy link
Contributor Author

Now I realize that the issue is rather in linkml-runtime and should be fixed there.

@dalito
Copy link
Collaborator

dalito commented Feb 19, 2024

I also had a look YYYYMMDD is never valid in jsonschema. So the test case cannot be generally changed just because py >= 3.11 is accepting another valid iso-format notation which is however not a valid internet time format.

@Silvanoc
Copy link
Contributor Author

The fix needs to be done in linkml-runtime and additional tests would be needed to avoid a regression. Pretty much the test that is failing here, but transfered to linkml-runtime.

@Silvanoc
Copy link
Contributor Author

Silvanoc commented Feb 19, 2024

I've added a regression test for the issue with the supported date formats to linkml/linkml-runtime#286 (which first fails before adding a fix) and then the needed change to the business logic. Once I get it merged into linkml-runtimeand a new release is available, we should be able to move forward on this PR. Until then, this PR will be on-hold.

Thanks to @dalito for his help on diagnosing the issue!

@sneakers-the-rat
Copy link
Collaborator

j2lyk typing_extensions is only needed for Literal in python<3.8, 3.8 has Literal, so if we are going >=3.8 then we should drop the dep and remove it from eg. pydanticgen, and well everywhere it's used!

@cmungall
Copy link
Member

Once I get it merged into linkml-runtimeand a new release is available, we should be able to move forward on this PR. Until then, this PR will be on-hold.

https://github.com/linkml/linkml-runtime/releases/tag/v1.7.2 is out, thanks for your help on this!

Python <3.8 needed 'typing_extension' to provide `Literal`. But since
Python <3.8 is not supported anymore, that dependency can be eliminated.

Signed-off-by: Silvano Cirujano Cuesta <silvano.cirujano-cuesta@siemens.com>
Signed-off-by: Silvano Cirujano Cuesta <silvano.cirujano-cuesta@siemens.com>
Signed-off-by: Silvano Cirujano Cuesta <silvano.cirujano-cuesta@siemens.com>
Restoring erroneously removed support for Pydantic v1.

Signed-off-by: Silvano Cirujano Cuesta <silvano.cirujano-cuesta@siemens.com>
@Silvanoc
Copy link
Contributor Author

Silvanoc commented Mar 6, 2024

Dropping pydantic1 support needs to be justified

@cmungall I've restored support for pydantic1 (55e51f6), since I dropped it due to a wrong assumption from my side.

@Silvanoc
Copy link
Contributor Author

Silvanoc commented Mar 6, 2024

Apparently there's still something missing to get it working with Pydantic v1 on Python 3.8. But I don't have currently any time to debug the issue...

What I can tell from the logs is that it's the Pydantic generator failing.

@sneakers-the-rat
Copy link
Collaborator

there ya go, just needed to merge in main. should be good to go now

@Silvanoc
Copy link
Contributor Author

there ya go, just needed to merge in main. should be good to go now

I don't know what you've done, because the merged commit introduced lots of changes and it's not a simply sync with 'main' (I've rebased and diffed to confirm it). In any case, the tests are green now.

@sneakers-the-rat
Copy link
Collaborator

sneakers-the-rat commented Mar 20, 2024

so we have the links on hand:

the parents of the commit are:

diffs:

what are you seeing that should be here and is missing? sorry if i messed up the merge, i literally just merged from main after pulling from both

go ahead and revert or whatever, but ya the error that was getting thrown last run was from something i fixed in main so whatever method of bringing us up to date there you prefer, that should fix it

also for the sake of recordkeeping: the bug here: #1959
was a symptom of this other PR: #1990
that now that i look at it this PR 1704 also does. i had been trying to remember when i had this problem before of a latent bug that only showed itself once the cache was cleared.

@Silvanoc
Copy link
Contributor Author

Silvanoc commented Mar 21, 2024

@sneakers-the-rat I never merge to synchronize, but rebased. Therefore I was puzzled by the amount of changes. If it's simply a sync with current main and it would be mergeable, it would be fine for me.

But trying to fix the conflicts mentioned in the PR is getting pretty tedious. I wonder if it's the double merging that is causing it. Or if it's me that I'm not used to sync-merge and then merge back...

Comment on lines 190 to 193
if sys.version_info >= (3, 8):
from typing import Literal
else:
from typing_extensions import Literal
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh my bad for missing this before! This is a comment demonstrating the usage and results of this object, not the template, so it's correct!

I pre-emptively removed typing_extensions from the DEFAULT_IMPORTS assuming this would be merged shortly after :) i'll fix rn

@sneakers-the-rat
Copy link
Collaborator

@sneakers-the-rat I never merge to synchronize, but rebased. Therefore I was puzzled by the amount of changes. If it's simply a sync with current main and it would be mergeable, it would be fine for me.

aha gotcha! ok glad that's cleared up, i got scared for a minute i had broken the whole thing <3

But trying to fix the conflicts mentioned in the PR is getting pretty tedious. I wonder if it's the double merging that is causing it.

just the same places in the code getting modified in main and here! but ya totally agreed, let's get this thing merged. dragged on for long enough that the changes have been whittled away in other PRs except for actually lifting the version cap!

just merged main again, so should b good to go.

@sneakers-the-rat
Copy link
Collaborator

ping on this, let's roll this out!

@Silvanoc
Copy link
Contributor Author

ping on this, let's roll this out!

I don't know whom you are trying to appeal. I'm not a maintainer and therefore I cannot merge. Even if I could, I wouldn't since the PR is blocked by a change request by @cmungall and he should give his green light. I have requested him to review it again, but I suppose that it's just one among multiple pending notifications...

@sneakers-the-rat
Copy link
Collaborator

i am pinging whoever can merge :) i was just working on a PR for linkml-runtime that tests against upstream linkml and wanted to include 3.12 in that, but can't since this hasn't been merged yet. so just friendly reminder in case it had fallen off whoever's radar is capable of merging this, no impatience implied.

@dalito
Copy link
Collaborator

dalito commented Mar 25, 2024

Chris has to merge. I can't.

@cmungall cmungall merged commit 9b39fdb into main Mar 26, 2024
17 checks passed
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.

None yet

4 participants