Skip to content

Code cleanups from pyupgrade #20642

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

Merged
merged 2 commits into from
Jul 31, 2025
Merged

Conversation

nsoranzo
Copy link
Member

Found by running:

make pyupgrade

The bulk of the changes are replacements of typing deprecated aliases such as Dict and List with the standard library classes. These aliases became redundant in Python 3.9 when the corresponding pre-existing classes were enhanced to support [], see https://docs.python.org/3/library/typing.html#deprecated-aliases

Also:

  • Update the pyupgrade Makefile target to use --py38-plus for the packages needed by Pulsar, including the new galaxy-tool-util-models package.
  • Use builtins.list in method return types inside classes that have list as a method.

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.

@nsoranzo
Copy link
Member Author

nsoranzo commented Jul 10, 2025

Git shortstat: +2,725 −3,534

@jmchilton
Copy link
Member

-0 - this change breaks the ability of older Galaxy instances to apply changes we make to their branches and is going to conflict big, open PRs while fixing no bugs and delivering no features.

Breaking compat with the little switches in the setting file that say what versions we support might make sense because CI things disappear or we need to bring in modern versions of dependencies but what is the purpose of this? It was fine - use the new syntax in new files if you want. I know the new syntax is better and I'm excited to use it someday and for new features, but at this point this seems like breaking things for the sake of breaking things.

@nsoranzo
Copy link
Member Author

-0 - this change breaks the ability of older Galaxy instances to apply changes we make to their branches and is going to conflict big, open PRs while fixing no bugs and delivering no features.

Breaking compat with the little switches in the setting file that say what versions we support might make sense because CI things disappear or we need to bring in modern versions of dependencies but what is the purpose of this? It was fine - use the new syntax in new files if you want. I know the new syntax is better and I'm excited to use it someday and for new features, but at this point this seems like breaking things for the sake of breaking things.

Thanks for looking at this! I understand this could be a concern, in fact I already experimented with this when working on #19685 but decided at the time to keep it out of that PR to make things smoother.
My counterarguments would be:

@nsoranzo nsoranzo force-pushed the pyupgrade_202507 branch 2 times, most recently from 6b99545 to 7ce31c1 Compare July 21, 2025 22:39
@nsoranzo nsoranzo force-pushed the pyupgrade_202507 branch 2 times, most recently from cafc811 to 101ddcc Compare July 30, 2025 11:33
@nsoranzo
Copy link
Member Author

Test failures unrelated.

@jmchilton
Copy link
Member

We went through various big code refactorings in the past (e.g. #454 , #11162 , #16954 , #17391 ). They bring of course a bit of pain in the short term, but we gain from having a more uniform and modern code base. I don't see why this one would be different.

  • I'm sure I was no fan of Python 3.8 as minimum #16954 at the time.
  • As for the rest - the difference between this and a generic black update is that I don't think the purpose of black is to break our code. If this was not breaking our code for older Pythons - I would evaluate it under the criteria of that. Those code formatting PRs are important but the purpose in some senses is to make the code uniform and easier to merge in and backport... doing too many increments though eliminates a lot of those advantages. This is excessive and break-y.

If we don't merge this, we cannot use pyupgrade --py39-plus on the codebase without having to manually cherry-pick the not-typing-aliases changes, loosing the benefits of this tool.

  • A tool that breaks our code and makes it work under fewer versions of Python is not useful IMO - I don't know why would break our code so we can make it easier to break in the future. They can deprecate Python as quickly as they want - they for the most part don't break existing code. There is no plan to ever do a Python 4 - I think we're safe. You do the PRs to support new releases though - so I am going to defer to you - it is a -0 not a -1.

A core tenant of this project is accessibility - I will applaud every "Add support for Python XX" PR and resist every "Drop support Python XX" PR. I feel like you sit in the same admin roundtables as me - non-usegalaxy.* galaxies are becoming less common and those that exist are many years out of date. We should be struggling to support them, not throwing them under the bus with this march toward making the code break on their Pythons.

Copy link
Member

@jmchilton jmchilton left a comment

Choose a reason for hiding this comment

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

This is a -0 approve since you requested my re-review - feel free to merge it I've stated my objections and I will do so with other PRs like this but I recognize I'm in the minority and frankly an old fart.

nsoranzo added 2 commits July 31, 2025 10:15
Found by running:

```
make pyupgrade
ruff check --fix .
ruff check --preview --unsafe-fixes --select=F401 --fix .
make format
make update-client-api-schema
```

The bulk of the changes are replacements of typing deprecated aliases
such as `Dict` and `List` with the standard library classes. These
aliases became redundant in Python 3.9 when the corresponding
pre-existing classes were enhanced to support `[]`, see
https://docs.python.org/3/library/typing.html#deprecated-aliases

Also:
- Update the `pyupgrade` Makefile target to use `--py38-plus` for the
  packages needed by Pulsar, including the new galaxy-tool-util-models
  package.
@nsoranzo
Copy link
Member Author

A core tenant of this project is accessibility - I will applaud every "Add support for Python XX" PR and resist every "Drop support Python XX" PR.

I don't think admins are the targets for accessibility, but even if we include them, I don't think that supporting an EOL Python version improves it.

I feel like you sit in the same admin roundtables as me - non-usegalaxy.* galaxies are becoming less common and those that exist are many years out of date. We should be struggling to support them, not throwing them under the bus with this march toward making the code break on their Pythons.

We do provide instructions on how to use a supported Python version, and it is pretty easy to install a new Python version nowadays. What would make small Galaxy admins' life easier is keeping old Galaxy releases supported forever, but I'm sure no one wants to do that!

Thanks anyway for taking the time for looking at this and commenting, appreciated!

@nsoranzo nsoranzo merged commit 90871e8 into galaxyproject:dev Jul 31, 2025
55 of 59 checks passed
@nsoranzo nsoranzo deleted the pyupgrade_202507 branch July 31, 2025 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants