Skip to content

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

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

Merged
merged 9 commits into from
Feb 11, 2020

Conversation

adrinjalali
Copy link
Member

Related to #14675.

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

@glemaitre
Copy link
Member

CI is failing

elif [[ "$DISTRIB" == "ubuntu" ]]; then
source $VIRTUALENV/bin/activate
pip install scikit-image
Copy link
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
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
Member Author

adrinjalali commented Sep 23, 2019

Why isn't the CI running? O_o

EDIT working

@adrinjalali
Copy link
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
Member

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

@adrinjalali
Copy link
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
Member

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

@adrinjalali
Copy link
Member Author

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

@thomasjpfan
Copy link
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
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
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
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
Member

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

@glemaitre
Copy link
Member

Thanks @thomasjpfan @adrinjalali

@glemaitre
Copy link
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