Skip to content

CI install scikit-image if we test doc on azure#15065

Merged
glemaitre merged 9 commits intoscikit-learn:masterfrom
adrinjalali:ci/skimage
Feb 11, 2020
Merged

CI install scikit-image if we test doc on azure#15065
glemaitre merged 9 commits intoscikit-learn:masterfrom
adrinjalali:ci/skimage

Conversation

@adrinjalali
Copy link
Copy Markdown
Member

Related to #14675.

This installs scikit-image whenever on azure we test the docs.

@glemaitre
Copy link
Copy Markdown
Member

CI is failing

conda install scikit-image -y
elif [[ "$DISTRIB" == "ubuntu" ]]; then
source $VIRTUALENV/bin/activate
pip install scikit-image
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you probably want to install scikit-image from the system

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

IIRC our min supported scikit-image is newer than the one available in ubuntu. And in install.sh we do install a bunch of libs in the environment, so I don't see why this should fail.

@adrinjalali
Copy link
Copy Markdown
Member Author

adrinjalali commented Sep 23, 2019

Why isn't the CI running? O_o

EDIT working

@adrinjalali
Copy link
Copy Markdown
Member Author

The system's skimage is too old, and I have no idea why this is not working. @thomasjpfan any hints?

@thomasjpfan
Copy link
Copy Markdown
Member

The quick solution is to only install skimage on instances with conda.

@adrinjalali
Copy link
Copy Markdown
Member Author

@thomasjpfan but that doesn't fix our issue, since I think one of the tests failing in #14675 is not a conda env.

@thomasjpfan
Copy link
Copy Markdown
Member

I am concerned with how fragile this is. I'll look into this.

@adrinjalali
Copy link
Copy Markdown
Member Author

thanks for the fixes @thomasjpfan , should we merge then?

@thomasjpfan
Copy link
Copy Markdown
Member

From my understanding this is for #14675. We would need to disable the doctest for all but one instance (by using doc/conftest.py). I think I am +0 on that.

@adrinjalali
Copy link
Copy Markdown
Member Author

From my understanding this is for #14675. We would need to disable the doctest for all but one instance (by using doc/conftest.py). I think I am +0 on that.

I'm not sure if I understand what exactly you mean @thomasjpfan

@thomasjpfan
Copy link
Copy Markdown
Member

We use https://github.com/scikit-learn/scikit-learn/blob/master/doc/conftest.py to skip doctest on some files depending if a package is installed. Adding scikit-image will add doc/tutorial/statistical_inference/unsupervised_learning.rst to this list of files to skip.

Now that I see that scikit-image is already a doc building dependency, I am okay +1 on this.

@adrinjalali
Copy link
Copy Markdown
Member Author

Thanks @thomasjpfan , @glemaitre you ok with this now?

@glemaitre glemaitre merged commit 933b4cf into scikit-learn:master Feb 11, 2020
@glemaitre
Copy link
Copy Markdown
Member

I am fine with it since this is in the doc building dependency

@glemaitre
Copy link
Copy Markdown
Member

Thanks @thomasjpfan @adrinjalali

@glemaitre
Copy link
Copy Markdown
Member

Let's finish up #14675

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.

3 participants