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.8 #6287

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

Conversation

gremlation
Copy link

No description provided.

@robinjhuang
Copy link
Collaborator

Can we just remove 3.8 from the CI? No need to break the code for no reason. @gremlation

@blepping
Copy link
Contributor

blepping commented Jan 3, 2025

Can we just remove 3.8 from the CI? No need to break the code for no reason.

What about future pull requests, would they still need to be compatible with 3.8?

@webfiltered
Copy link
Collaborator

webfiltered commented Jan 3, 2025

What about future pull requests, would they still need to be compatible with 3.8?

PRs no longer need to be compatible with 3.8.

We just wanted to avoid explicitly removing 3.8 compat from existing code right now, if there is no measurable gain from doing so (it is a very small workaround). Happy to retract that if there's some performance / other benefit I've totally missed.

@gremlation
Copy link
Author

The point of this is to drop support. I recently worked on some Comfy code. Then I realised I didn't know what version of Python I needed to support because ComfyUI doesn't document it anywhere. I did some digging and found that CI is configured for 3.8. So I needed to go back and change my code to work with 3.8.

This is a waste of time. I could have been working on a new feature, writing docs, or fixing bugs. Instead I was adding compatibility for a dead version of Python virtually nobody uses. This is the opportunity cost of supporting old versions.

3.8 is dead. Not even the Python org supports 3.8 any more. 3.9, 3.10, and 3.11 are all in security fix only mode, and no new binaries are being released even when security holes are found. Only 3.12+ is fully supported by the Python org.

The VFX reference platform specified 3.9 back in 2022, 3.10 in 2023, and 3.11 in 2024. Software compatible with this can't be used on the old versions of Python. A lot of software in general can't.

What ComfyUI is doing is wildly out of step with what everybody else is doing and supporting old versions wastes development time. Development time should be spent on the huge majority of people using a normal version of Python, not the tiny sliver of people using an obsolete version. Dropping 3.8 is really the absolute minimum, it would be far more sensible to be more aggressive in dropping more versions at the same time.

@blepping
Copy link
Contributor

blepping commented Jan 3, 2025

Unless I am misunderstanding, the people that responded so far support the pull. The only thing they were saying to let it break organically when there's an actual need rather than rushing to make sure that 3.8 users crash the instant support is dropped.

Does it really make a difference to do it that way? Right now, a week from now, a month from now - it's going to happen once support is dropped. I'd say probably not, but also it's not really an important thing to oppose if some people prefer to do it that way.

At least in my opinion the two main things are 1) Making it clear what Python version future changes need to target, and 2) freeing those future changes from the burden of trying to support Python 3.8. If this pull gets accepted, then you succeeded in doing that even if lcm gets to survive for another month.

@gremlation
Copy link
Author

gremlation commented Jan 7, 2025

Leaving old code in "just in case" is a source of bugs. In this particular case, ComfyUI's implementation of lcm() isn't as complete as Python's implementation of lcm(). So if anybody used lcm() thinking it was the real thing, it might not work right. As a general principle, dropping support for a version but leaving all the code in to support it is functionally equivalent to not dropping support, but with the added confusion of nobody is really sure what the situation is.

Either Python 3.8 is supported, in which case this PR should be closed, or Python 3.8 is not supported, in which case removal of tech debt is good. An indecisive gray area is the worst of both worlds. What is needed is clarity.

Also, I asked in Discord, but it's worth repeating here: can somebody who has permission update How to Contribute Code to state which versions of Python are supported please? It really sucks to discover you've written incompatible code that can't be merged.

@huchenlei
Copy link
Collaborator

Unless I am misunderstanding, the people that responded so far support the pull. The only thing they were saying to let it break organically when there's an actual need rather than rushing to make sure that 3.8 users crash the instant support is dropped.

Does it really make a difference to do it that way? Right now, a week from now, a month from now - it's going to happen once support is dropped. I'd say probably not, but also it's not really an important thing to oppose if some people prefer to do it that way.

At least in my opinion the two main things are 1) Making it clear what Python version future changes need to target, and 2) freeing those future changes from the burden of trying to support Python 3.8. If this pull gets accepted, then you succeeded in doing that even if lcm gets to survive for another month.

The min python version will be tracked in pyproject.toml. See #6386

@huchenlei
Copy link
Collaborator

Leaving old code in "just in case" is a source of bugs. In this particular case, ComfyUI's implementation of lcm() isn't as complete as Python's implementation of lcm(). So if anybody used lcm() thinking it was the real thing, it might not work right. As a general principle, dropping support for a version but leaving all the code in to support it is functionally equivalent to not dropping support, but with the added confusion of nobody is really sure what the situation is.

Either Python 3.8 is supported, in which case this PR should be closed, or Python 3.8 is not supported, in which case removal of tech debt is good. An indecisive gray area is the worst of both worlds. What is needed is clarity.

Also, I asked in Discord, but it's worth repeating here: can somebody who has permission update How to Contribute Code to state which versions of Python are supported please? It really sucks to discover you've written incompatible code that can't be merged.

We posted decision on dropping 3.8 support in #6401 and we shall merge this PR on 2025-01-22.

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.

6 participants