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

Drop support for python < 3.6 #228

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Drop support for python < 3.6 #228

wants to merge 1 commit into from

Conversation

pgiraud
Copy link
Member

@pgiraud pgiraud commented Nov 13, 2024

No description provided.

@rjuju
Copy link
Member

rjuju commented Nov 14, 2024

I'm quite opposed to this. What problem does it solve? It only creates new problem, as the target for powa is not a web developer running some distro with all the packages in bleeding edge version but companies running postgres on sometimes somewhat legacy systems, usually without any possibility to upgrade anything. I'm still annoyed that we don't support python 2 anymore but ok it's really dead. But I'm pretty sure that no one is going to thank us for dropping support for python 3.9-. Even our demo still uses an older version than that.

@rjuju
Copy link
Member

rjuju commented Nov 16, 2024

again, why aggressively dropping support for python versions that users might be using?

I spent a bit of time trying to figure out what versions are now supported, since you didn't document it. It seems to me that python 3.3 would work. It looks like it's only broken by ruff that adds trailing commas everywhere. I'd rather remove that behavior and restore support down to python 3.3.

@pgiraud
Copy link
Member Author

pgiraud commented Nov 18, 2024

I don't think python 3.3 should fail on trailing comas. Can you please give example of code that breaks compatibility with python 3.3?

@rjuju
Copy link
Member

rjuju commented Nov 18, 2024

You can reproduce easily by spawning a python:3.3 container and bind mounting powa-web. Doing so:

root@236b65caeb0f:/usr/local/src/powa/powa-web# ./run_powa.py 
Traceback (most recent call last):
  File "./run_powa.py", line 4, in <module>
    from powa import make_app
  File "/usr/local/src/powa/powa-web/powa/__init__.py", line 132
    **kwargs,
            ^
SyntaxError: invalid syntax

after removing the trailing comma it fails somewhere else, so I'm assuming it's the problem:

root@236b65caeb0f:/usr/local/src/powa/powa-web# ./run_powa.py 
Traceback (most recent call last):
  File "./run_powa.py", line 4, in <module>
    from powa import make_app
  File "/usr/local/src/powa/powa-web/powa/__init__.py", line 24, in <module>
    from powa.collector import (
  File "/usr/local/src/powa/powa-web/powa/collector.py", line 9, in <module>
    from powa.dashboards import MetricGroupDef
  File "/usr/local/src/powa/powa-web/powa/dashboards.py", line 529
    **kwargs,
            ^
SyntaxError: invalid syntax

@pgiraud
Copy link
Member Author

pgiraud commented Nov 19, 2024

Indeed, the following is invalid for Python < 3.6 (see https://bugs.python.org/issue9232).:

def foo(bar, dude,): # valid
    [...]

def foo(bar, **kwargs,): # invalid
    [...]

What problem does it solve?

First, I would like to apologize for the commits that introduced the formatting changes with Ruff. I'm sorry I didn't pay enough attention to the target python version, didn't test meticulously and more importantly didn't document the change.

By the way, it's not really a Ruff problem. I could have used Black with similar results.

I doesn't really solve any problem. Not in the way the tool actually works. I was just trying to make sure that the code style is consistent and modern for the sake of developers but not only. See below.

Why aggressively dropping support for python versions that users might be using?

How do we know that the current users won't be able to use PoWA because of compatibility issues? This seems very hypothetical. I'd be curious to know how many users will want to upgrade PoWA and are not able to install a relatively recent version of Python (> 3.6).

The target for powa is not a web developer running some distro with all the packages in bleeding edge version

Python follows a clear release cycle. Just like PostgreSQL does.
https://devguide.python.org/versions/

Python 3.5 was released in 2015-09-13 and reached end of life in 2020-09-30. While I agree that 3.9 was a bit too restrictive, I think keeping support for 3.6 is reasonable.

Keeping PoWA compatible with old versions of Python gives the project a dusty image, which can have the effect of slowing down outside contributions, whether new or old. Potential contributors may already be reluctant to contribute to the project if the code isn't modernized or if proposals for modernization are rejected.

If the Python language evolves, it's for the good of its users, who can then write more secure/maintainable/readable/... code. If our project's code itself doesn't evolve, we are leaving developers to work in an environment that's uncomfortable because it's not very reassuring, or even counter-productive given that they have to constantly focus on compatibility to the detriment of the rest, i.e. what really matters. Missing tests is also a bad sign.

Don't we want to see new motivated contributors join the project? Or do we want to keep the compatibility for a hypothetical user base?

Also, we probably don't want users to run PoWA on versions of Python that are not maintained for security reasons. Generally, it's a good thing to advise users to keep their system up-to-date. And up-to-date doesn't necessarily mean bleeding edge.

Could we please at least accept that Python 3.6 (preferably 3.7) is the minimal version PoWA can be run onto? That would already be an appreciated step forward.

@rjuju
Copy link
Member

rjuju commented Nov 19, 2024

Indeed, the following is invalid for Python < 3.6 (see https://bugs.python.org/issue9232).

Ah I see, the difference is subtle (at least for a non python developer like me) :)

First, I would like to apologize for the commits that introduced the formatting changes with Ruff. I'm sorry I didn't pay enough attention to the target python version, didn't test meticulously and more importantly didn't document the change.

No worries, fortunately the UI in this project is quite disconnected from the rest so it's really easy to release new versions frequently and it's not really a problem to apply them on the client side (assuming they do upgrade).

By the way, it's not really a Ruff problem. I could have used Black with similar results.

Yes totally agreed. I'm assuming it's just implementing some PEP recommendations.

How do we know that the current users won't be able to use PoWA because of compatibility issues? This seems very hypothetical. I'd be curious to know how many users will want to upgrade PoWA and are not able to install a relatively recent version of Python (> 3.6).

In my view that should be the opposite: how can you guarantee that no one will ever need to update powa using some older python version?

But to answer your question, after working for decades around databases, reading discussion on the various mailing list and so on I see all the time people using very old versions of postgres, and I'm not talking about only the minor version of postgres. And usually those people just stick with the same version of what they originally installed: postgres, OS and everything else. That's why at least RHEL has very long extended support.

I agree that cloud providers and hosted solutions won't have this problem, but they also won't use or provide powa either.

Python follows a clear release cycle. Just like PostgreSQL does.
https://devguide.python.org/versions/

Python 3.5 was released in 2015-09-13 and reached end of life in 2020-09-30. While I agree that 3.9 was a bit too restrictive, I think keeping support for 3.6 is reasonable.

Note that powa-archivist still support postgres 9.5, released in 2016 and EOLed in 2021. That's why I think that the front-end should be compatible with version of python that roughly matches the same interval.

Keeping PoWA compatible with old versions of Python gives the project a dusty image, which can have the effect of slowing down outside contributions, whether new or old. Potential contributors may already be reluctant to contribute to the project if the code isn't modernized or if proposals for modernization are rejected.

In my experience if people don't want to contribute to something they will always find good reasons not to. On the other hand if they do want to contribute they're unlikely to be driven off by something like that, especially if the CI is there to catch syntax errors / older version compatibility.

If the Python language evolves, it's for the good of its users, who can then write more secure/maintainable/readable/... code. If our project's code itself doesn't evolve, we are leaving developers to work in an environment that's uncomfortable because it's not very reassuring, or even counter-productive given that they have to constantly focus on compatibility to the detriment of the rest, i.e. what really matters. Missing tests is also a bad sign.

Don't we want to see new motivated contributors join the project? Or do we want to keep the compatibility for a hypothetical user base?

Whether we raise the minimum python version or not will not change the fact that the code will still be complex, full of over lengthy queries that have to deal with multiple versions of postgres, multiple combinations of extensions and so on. That's far more likely to discourage anyone from working on this project than not being able to use whatever new syntax that would now be allowed. Unfortunately what this is doing is actively preventing me from contributing to this project since I don't really understand newer python code (I'm already entirely unable to do anything on the js side). I'm not a python developer (which likely explains a lot of the weird code here) and haven't used python at work for years, so I don't really have any opportunity (or time) to keep up with it.

Also, we probably don't want users to run PoWA on versions of Python that are not maintained for security reasons. Generally, it's a good thing to advise users to keep their system up-to-date. And up-to-date doesn't necessarily mean bleeding edge.

While I totally agree that people shouldn't run EOL / unmaintained code, I don't think that it's our place to force them to update. Again, in my experience people don't run scary old versions of stuff because they like it, but because they simply don't have a choice. So if they happened to want/need powa, preventing them from using it would not help :(

Could we please at least accept that Python 3.6 (preferably 3.7) is the minimal version PoWA can be run onto? That would already be an appreciated step forward.

I'm ok with 3.6 (preferably), as long as you're the one dealing with the hypothetical annoyed users.

@pgiraud pgiraud marked this pull request as draft November 19, 2024 15:22
@pgiraud pgiraud force-pushed the pyupgrade branch 7 times, most recently from d16f2ef to 4471cca Compare November 20, 2024 08:45
The Python syntax is upgrade accordingly by enabling
pyupgrade in Ruff configuration.

The following commands were executed:
 - `ruff check --fix`
 - `ruff check --fix --unsafe-fixes`
 - `ruff format`

We added python 3.7 in the lint tests job in CI. And we also check for
obvious syntax errors on Python3.6.
@pgiraud pgiraud changed the title Upgrade python syntax and drop support for unsupported versions of python Drop support for python < 3.6 Nov 20, 2024
@pgiraud pgiraud marked this pull request as ready for review November 20, 2024 09:00
@pgiraud
Copy link
Member Author

pgiraud commented Nov 22, 2024

PR updated. Can you please have look?
After doing a 5.0.1 release with the different fixes, of course.

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.

2 participants