-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
CI install pillow in Travis cron job #13080
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
Conversation
Why should we install master? What made it suddenly fail? |
IMO pillow should not be a mandatory dependency for doctests. Is there a way to conditionally skip doctests ? |
Some tests also rely on pillow, installing pillow in cron job will help us discover possible bugs earlier. |
Hmm, also I don't understand how CI passed in #12819 if pillow was missing and it added an example that required it... |
All our CI seem to install pillow?
|
Nevermind, I see it's installed in all CI.
Agreed, my point is that checking if pillow is installed and then running those tests sounds fine. Having some test fail if pillow is not installed is IMO not great. I mean a mandatory test dependency is not as bad as a runtime dependency, but it also comes with some of the same constraints and costs. Also, for instance, we are not requiring pandas for tests (which is an incresingly important part of our API tests) so I'm not sure why should we should require pillow that is only used in a few marginal tests. |
Depends what others think about this, but I would be +1 to merge this, but then not install pillow in one of the other Travis CI jobs (in some other PR). |
Yes, I agree.
|
I guess there's consensus to merge? |
This reverts commit 25706c2.
This reverts commit 25706c2.
Resolves Travis cron job failure in master.
See https://travis-ci.org/scikit-learn/scikit-learn/builds/488111910
We install pillow in other Travis jobs (and other CIs), so I think it's reasonable to install it in Travis cron job.