8000 MAINT Vendors packaging/version.py for pep440 versioning by thomasjpfan · Pull Request #19826 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

MAINT Vendors packaging/version.py for pep440 versioning #19826

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Apr 19, 2021
Merged

MAINT Vendors packaging/version.py for pep440 versioning #19826

merged 6 commits into from
Apr 19, 2021

Conversation

thomasjpfan
Copy link
Member

Reference Issues/PRs

Closes #19102
Fixes #19098

What does this implement/fix? Explain your changes.

Vendors out packaging/version.py to use their parse_version. This is what scipy does as well.

CC @rth

@rth
Copy link
Member
rth commented Apr 6, 2021

Thanks! How did you obtain that file? Keeping it in sync with upstream will be harder if it doesn't correspond to individual upstream files. Of are too many other things in packaging we don't want to include?

@thomasjpfan
Copy link
Member Author

The files were obtained through https://github.com/pypa/packaging/tree/main/packaging. We only need the _structures.py and version.py files.

I updated the PR to vendor the two files directly, so it would be easier to keep in sync.

rth
rth approved these changes Apr 7, 2021
View reviewed changes
Copy link
Member
@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM, assuming the CI passes (currently there are some CI failures).

@ogrisel
Copy link
Member
ogrisel commented Apr 7, 2021

The windows CI fails, probably because we do not properly install the new submodule when installing scikit-learn to site-packages:

    from ..externals._packaging.version import parse as parse_version
E   ModuleNotFoundError: No module named 'sklearn.externals._packaging'

@ogrisel
Copy link
Member
ogrisel commented Apr 7, 2021

I think we need a:

    config.add_subpackage('externals/_packaging')

in sklearn/setup.py.

ogrisel
ogrisel approved these changes Apr 7, 2021
View reviewed changes
Copy link
Member
@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM as well assuming the packaging issues mentioned above is solved.

@adrinjalali
Copy link
Member
adrinjalali commented Apr 18, 2021 •
edited
Loading

Do we want setup.py to use the vendored version or directly importing from pkg_resources? Right now it doesn't use our copy

@thomasjpfan
Copy link
Member Author

For setup.py, it is not a big issue because pkg_resources is used for build time. Vendoring was to help with the slow import during runtime.

On the other hand, I am slightly in favor of using the vendored version in setup.py to remove all imports of pkg_resources so future contributors can easily tell to use the vendored version. (Updated PR to reflect this)

@adrinjalali
Copy link
Member

All the tests have passed, but there's a cross sign on your commit, why?

@thomasjpfan
Copy link
Member Author

@adrinjalali Since I changed setup.py, I ran [cd build] to test wheel building which includes ARM. I wanted to conserve travis CI ARM credits, so I canceled 3 out of 4 jobs and let one fully run. The canceled travis jobs caused the negative check.

XREF: travis job results

@adrinjalali adrinjalali merged commit 5efcb10 into scikit-learn:main Apr 19, 2021
thomasjpfan added a commit to thomasjpfan/scikit-learn that referenced this pull request Apr 19, 2021
@thomasjpfan
MAINT Vendors packaging/version.py for pep440 versioning (scikit-lear…
589b8fe 
…n#19826)

* MAINT Vendors packaging/version for pep440 versioning

* ENH Uses folder structure of packaging

* ENH Uses fixes parse_version

* ENH Uses packaging name

* MAINT Adds packaging

* BLD Use vendored version [cd build]
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
Merged
12 tasks
@QuentinSoubeyran QuentinSoubeyran mentioned this pull request Oct 5, 2021
Merged
@LilianBoulard LilianBoulard mentioned this pull request Nov 14, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

@rth rth rth approved these changes

@adrinjalali adrinjalali adrinjalali approved these changes

Assignees
No one assigned
Labels
module:utils No Changelog Needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Avoid importing pkg_resources to speed up import
4 participants
@thomasjpfan @rth @ogrisel @adrinjalali
0