Skip to content

MNT pin PyWavelet in doc-min builds #21607

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 4 commits into from
Nov 12, 2021
Merged

MNT pin PyWavelet in doc-min builds #21607

merged 4 commits into from
Nov 12, 2021

Conversation

glemaitre
Copy link
Member

@glemaitre glemaitre commented Nov 9, 2021 •
edited
Loading

PyWavelets 1.2.0 requires numpy >= 1.17 that is not has a known bug with pandas <= 1.2.
Pinning the version of PyWavelets would be enough here.

@glemaitre glemaitre marked this pull request as draft November 9, 2021 13:57
@glemaitre
Copy link
Member Author

OK. So the thing that is fishy here is:

@jorisvandenbossche mentioned that it could be something linked with the latest release of setuptools. I will downgrade to investigate this.

@glemaitre
Copy link
Member Author

Actually, it might only be some pip printing misleading. PyWavelets have been updated from 1.1.1 to 1.2.0 which probably change their minimal NumPy dependency.

@glemaitre
Copy link
Member Author

I opened pypa/pip#10651 to report the misleading log given by pip

@glemaitre glemaitre marked this pull request as ready for review November 10, 2021 13:52
@glemaitre glemaitre changed the title MNT check doc_min_build failure MNT pin PyWavelet in doc-min builds Nov 10, 2021
@glemaitre
Copy link
Member Author

pinging @adrinjalali @thomasjpfan

adrinjalali
adrinjalali reviewed Nov 10, 2021
View reviewed changes
build_tools/circle/build_doc.sh
Comment on lines +148 to +149
wget https://github.com/conda-forge/miniforge/releases/latest/download/Mambaforge-Linux-x86_64.sh \
-O miniconda.sh
Copy link
Member

Choose a reason for hiding this comment

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

O_o do we want to switch to mamba? why not miniforge?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was for debugging reasons first. Then, I assume that mamba will be faster than conda to resolve the dependencies required here.

build_tools/circle/build_doc.sh
# NumPy version required. If PyWavelets 1.2+ is installed, it would require
# NumPy 1.17+ that trigger a bug with Pandas 0.25:
# https://github.com/numpy/numpy/issues/18355#issuecomment-774610226
pip install PyWavelets==1.1.1
Copy link
Member

Choose a reason for hiding this comment

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

we need a comment here, or in our min dependency file, which would remind us to remove this once we bump our min numpy requirement.

adrinjalali
adrinjalali approved these changes Nov 10, 2021
View reviewed changes
Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

LGTM, and the circleCI passes.

thomasjpfan
thomasjpfan approved these changes Nov 12, 2021
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM to unblock other PRs

For the future, I would prefer to remove the skimage dependency from our examples and docs.

@thomasjpfan thomasjpfan merged commit bbb81e9 into scikit-learn:main Nov 12, 2021
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Nov 22, 2021
@glemaitre
MNT pin PyWavelet in doc-min builds (scikit-learn#21607)
e09eaab
@sply88 sply88 mentioned this pull request Nov 27, 2021
Merged
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Nov 29, 2021
@glemaitre
MNT pin PyWavelet in doc-min builds (scikit-learn#21607)
02d7332
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
@glemaitre @samronsin
MNT pin PyWavelet in doc-min builds (scikit-learn#21607)
8fc2b59
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Dec 24, 2021
@glemaitre
MNT pin PyWavelet in doc-min builds (scikit-learn#21607)
e296a43
glemaitre added a commit that referenced this pull request Dec 25, 2021
@glemaitre
MNT pin PyWavelet in doc-min builds (#21607)
d43c758
@thomasjpfan thomasjpfan mentioned this pull request Mar 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrinjalali adrinjalali adrinjalali approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@glemaitre @adrinjalali @thomasjpfan