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

Updated distutils.Version to packaging.Version #3897

Merged
merged 2 commits into from Jan 31, 2023

Conversation

hoxbro
Copy link
Contributor

@hoxbro hoxbro commented Sep 22, 2022

Code PR

  • I have read through the contributing notes and understand the structure of the package. In particular, if my PR modifies code of plotly.graph_objects, my modifications concern the codegen files and not generated files.
  • I have added tests (if submitting a new feature or correcting a bug) or
    modified existing tests.
  • For a new feature, I have added documentation examples in an existing or
    new tutorial notebook (please see the doc checklist as well).
  • I have added a CHANGELOG entry if fixing/changing/adding anything substantial.
  • For a new feature or a change in behaviour, I have updated the relevant docstrings in the code to describe the feature or behaviour (please see the doc checklist as well).

Fixes #3893

@astrojuanlu
Copy link
Contributor

Looks like the CI failures were unrelated, although some time has passed and it would be good to rebase this I think. Any other reason to not merge it? If @hoxbro is not interested in pursuing this, I'll be happy to take over.

@nicolaskruchten
Copy link
Member

This seems like an interesting PR but I'd like more information about what problems it solves and what risks it entails... What concrete problems are being experienced with the current code? Is the replacement a well-tested, drop-in replacement for the deprecated one, or are there caveats we need to be aware of and check? Why is the old thing being deprecated in favour of the new thing?

@astrojuanlu
Copy link
Contributor

Long story short, distutils was deprecated in Python 3.10 https://peps.python.org/pep-0632/ and is slated for removal in Python 3.12 python/cpython#99061

The "Migration Advice" section of PEP 632 lists packaging as a replacement of distutils.version.

@nicolaskruchten
Copy link
Member

Thanks for the explanation! So packaging.version.Version is the new thing that replaces LooseVersion or is there some semantic difference between the two?

@nicolaskruchten
Copy link
Member

(I believe the build failure is indeed unrelated, or at least can be fixed elsewhere, so the only thing blocking a merge here is my understanding of the change :)

@astrojuanlu
Copy link
Contributor

Funny, distutils.version documentation is empty :) https://docs.python.org/3.11/distutils/apiref.html#module-distutils.version

They're not the same, but for the purposes that matter to us, they are. distutils.version.LooseVersion describes itself as "Version numbering for anarchists and software realists." and mentions that "there is no such thing as an invalid version number under this scheme". On the other hand, packaging.version.Version is PEP 440 compatible, so there are some invalid versions https://peps.python.org/pep-0440/

@nicolaskruchten
Copy link
Member

Lol! I do wonder if moving to something stricter might not cause some hard to diagnose breaking changes... Maybe migrating to something that claims to be compatible might be a better choice? Something like https://pypi.org/project/looseversion/ ?

@astrojuanlu
Copy link
Contributor

There's a tiny possibility of breaking changes, but for that to happen an invalid version of ipywidgets, matplotlib, or orca should reach the user environment, which is highly unlikely because PyPI disallowed invalid versions a long time ago.

On the other hand, in the event of a breaking change, it wouldn't be hard to diagnose at all: a loud InvalidVersion error would be raised.

I see how adopting looseversion is the safest approach, but on the other hand the ecosystem has solidly moved towards PEP 440 compliant versions and the chance of breakage is minimal.

Either way, removing the distutils dependency is the important task, so if the devs are not sure about switching to packaging, we might as well add looseversion as a dependency. The alternative is broken installs with Python 3.12.

@nicolaskruchten
Copy link
Member

OK. Thanks @astrojuanlu !

@nicolaskruchten
Copy link
Member

And thanks @hoxbro for the PR :)

@nicolaskruchten nicolaskruchten merged commit 92ce5bc into plotly:master Jan 31, 2023
@nicolaskruchten
Copy link
Member

Hehe so I just realized that this PR modified auto-generated code... I'll need to modify the source of that code-gen process for these changes to stick: https://github.com/plotly/plotly.py/blob/master/packages/python/plotly/codegen/__init__.py#L273

@hoxbro hoxbro deleted the change_version branch February 3, 2023 21:56
@nicolaskruchten
Copy link
Member

@astrojuanlu I didn't check on this earlier but... it seems like packaging is not available for Python 3.6, which is still meant to be supported by plotly. Do you have a recommendation for moving forward here? What do other libraries do?

@nicolaskruchten
Copy link
Member

Ah it seems like older versions of packaging work on 3.6, actually so I guess just adding packaging to setup.py as a dependency will be enough.

@astrojuanlu
Copy link
Contributor

That's right: pypa/packaging#500 I think pip will pick up the right version in this case

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.

LooseVersion is deprecated, use packaging.version instead
3 participants