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 #16796

Merged
merged 6 commits into from
Jan 22, 2024
Merged

Add support for Python 3.12 #16796

merged 6 commits into from
Jan 22, 2024

Conversation

tuncK
Copy link
Contributor

@tuncK tuncK commented Oct 6, 2023

Updated Python 3.11 requirements for the new Python 3.12 release during the EGD 2023.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@github-actions github-actions bot added this to the 23.2 milestone Oct 6, 2023
@nsoranzo nsoranzo changed the title Add support for python3.12 Add support for Python 3.12 Oct 6, 2023
@nsoranzo nsoranzo added area/python3 Specific to Python 3 kind/feature labels Oct 6, 2023
@nsoranzo
Copy link
Member

@tuncK I've rebased this to fix conflicts in the dependencies, hope that's OK.

@tuncK
Copy link
Contributor Author

tuncK commented Oct 10, 2023

@tuncK I've rebased this to fix conflicts in the dependencies, hope that's OK.

Feel free, I am swamped and could not work on this yet.

@nsoranzo
Copy link
Member

nsoranzo commented Oct 10, 2023

Uninstallable dependencies:

  • aiohttp: Python 3.12 will be supported by 3.9 ** 3.9 Beta testing ** aio-libs/aiohttp#7675 , but Python 3.7 support is dropped in the same release.
  • frozenlist: Python 3.12 support was added in 1.4.0 , but Python 3.7 support was dropped in the same release.
  • greenlet: 2.0.2 strictly required by playwright 1.35.0 (last release supporting Python 3.7). playwright 1.39.0 (first release supporting Python 3.12) has strict pins on greenlet 3.0.0 and pyee 11.0.1, making it impossible to use our current split_requirement trick
  • h5py: Python 3.12 support was added in 3.10 , but Python 3.7 support was dropped in 3.9
  • uvloop: Needs a new release with Port uvloop to Python 3.12 MagicStack/uvloop#570 included.

Errors:

@nsoranzo nsoranzo force-pushed the py3_12 branch 2 times, most recently from d18caa0 to 118b38a Compare October 14, 2023 20:38
@nsoranzo
Copy link
Member

greenlet 2.0.2 strictly required by playwright 1.35.0 (last release supporting Python 3.7). playwright 1.39.0 (first release supporting Python 3.12) has strict pins on greenlet 3.0.0 and pyee 11.0.1, making it impossible to use our current split_requirement trick

Based on this I am inclined to say this depends on dropping support for Python 3.7 #16394 .

@mvdbeek
Copy link
Member

mvdbeek commented Oct 18, 2023

We ... could freeze next week for 23.2 and drop 3.7 after that. How do you feel about that @dannon ?

@nsoranzo nsoranzo force-pushed the py3_12 branch 2 times, most recently from 6f8b648 to 6088676 Compare October 19, 2023 11:36
@nsoranzo nsoranzo mentioned this pull request Oct 19, 2023
4 tasks
@jmchilton
Copy link
Member

greenlet 2.0.2 strictly required by playwright 1.35.0 (last release supporting Python 3.7). playwright 1.39.0 (first release supporting Python 3.12) has strict pins on greenlet 3.0.0 and pyee 11.0.1, making it impossible to use our current split_requirement trick

hoisted on my own petard 😿

@nsoranzo nsoranzo force-pushed the py3_12 branch 2 times, most recently from 10bc5a9 to c7a12fd Compare December 17, 2023 19:27
@mvdbeek
Copy link
Member

mvdbeek commented Dec 18, 2023

That's going to be a tough one if it's not going to be fixed upstream. We rely on this for old tools with python2 cheetah syntax to work. I have no idea how widespread use of these tools is at this point. We could in principle serialize the job inputs and use a container to build the command line, but that's going to be tricky.

@mvdbeek mvdbeek modified the milestones: 23.2, 24.0 Dec 19, 2023
@nsoranzo
Copy link
Member

That's going to be a tough one if it's not going to be fixed upstream. We rely on this for old tools with python2 cheetah syntax to work. I have no idea how widespread use of these tools is at this point. We could in principle serialize the job inputs and use a container to build the command line, but that's going to be tricky.

Yes, I think this may be the last obstacle for Python 3.12 support. Unfortunately future looks abandoned (last commit in January 2023 and no response to the above issue or to a WIP PR partially fixing it).
Additionally, lib2to3 (which we also in lib/galaxy/util/template.py for the Cheetah conversion) is deprecated and will probably be removed in 3.13.

So I think we need to decide among these 3 options:

  • fork the python-future repo, fix the issue with using the removed imp module, and update our requirements to install from there. Think about lib2to3 when it's actually removed;
  • vendorize the parts of python-future (and later lib2to3) that we actually use;
  • Drop the support for Python 2 in Cheetah.

Let me know what you think.

@mvdbeek
Copy link
Member

mvdbeek commented Jan 11, 2024

Drop the support for Python 2 in Cheetah.

I think that'd be the last resort, the other two options seem fine with me ?

I know we're just postponing the inevitable there, but the further way we are form when we last wrote tools that used python 2 syntax, the better.

@nsoranzo
Copy link
Member

For the future dependency, I've opted for option 1 ("fork the python-future repo, fix the issue with using the removed imp module, and update our requirements to install from there"), see https://github.com/nsoranzo/python-future/tree/python312-imp-module_past for the branch with my fixes.

I am not sure about the remaining 3 failing unit tests:

FAILED test/unit/util/test_fill_template.py::test_fill_list_comprehension_template - Failed: DID NOT RAISE <class 'NameMapper.NotFound'>
FAILED test/unit/util/test_fill_template.py::test_fill_dict_comprehension - Failed: DID NOT RAISE <class 'NameMapper.NotFound'>
FAILED test/unit/util/test_fill_template.py::test_set_comprehension - Failed: DID NOT RAISE <class 'NameMapper.NotFound'>

Suggestions welcome.

@mvdbeek
Copy link
Member

mvdbeek commented Jan 21, 2024

I think these are present as a sort of baseline to "justify" the fixes we're applying and were added in 85e13d2

From https://docs.python.org/dev/whatsnew/3.12.html

Calling locals() inside a comprehension now includes variables from outside the comprehension, and no longer includes the synthetic .0 variable for the comprehension “argument”.

is breaking (or actually fixing) this. Now that will result in some minor potential differences between what works on python 3.12 and python < 3.12, but I think that's ok. My suggestion would be to remove the tests and keep testing tools on 3.11 until 3.11 is sunset.

This is really awesome, thanks for the work @nsoranzo and @tuncK

Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

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

Do you want to cut a release of your fork before merging ?

@nsoranzo
Copy link
Member

From https://docs.python.org/dev/whatsnew/3.12.html

Calling locals() inside a comprehension now includes variables from outside the comprehension, and no longer includes the synthetic .0 variable for the comprehension “argument”.

is breaking (or actually fixing) this. Now that will result in some minor potential differences between what works on python 3.12 and python < 3.12, but I think that's ok.

Well spotted! I checked the 3.12 release notes but I was looking for changes related to the PEG parser.

My suggestion would be to remove the tests and keep testing tools on 3.11 until 3.11 is sunset.

I've updated the tests to work differently on Python 3.12 and later.

Do you want to cut a release of your fork before merging ?

For the time being I'd prefer to keep pointing at the fork branch, tagging a release sounds like picking up maintainership. Poetry translates this to a commit id any way in lib/galaxy/dependencies/pinned-requirements.txt .

@mvdbeek mvdbeek merged commit 6d58253 into galaxyproject:dev Jan 22, 2024
54 checks passed
@mvdbeek mvdbeek added the highlight/admin Included in admin/dev release notes label Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants