-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Install matplotlib before test dependencies on Azure #19353
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
Hmm, doesn't look like this has worked. |
This seems to work now - the build is failing after 4 minutes instead of 1hr. |
I am confused by why CI is still marked as failed, though. There's the |
I deliberately removed that at the install stages, so if installing fails the build fails. I'm not too sure what the logic is for spending time on a build that always passes by construction, and therefore doesn't provide helpful feedback on whether anything is broken or not? If the motivation is not to block PRs on a failing python dev, wouldn't it be best to run it only on the |
Ah, I somehow completely missed the fact that you had removed it. |
Well, I think this is an improvement as it changes the python pre CI to failing after 60 minutes with a timeout to failing after 4 minutes. I'd say we should merge this, and discuss what to do long term with the python pre build on a new issue. |
python -m pip install -ve . || | ||
[[ "$PYTHON_VERSION" = 'Pre' ]] | ||
displayName: "Install self" | ||
python -m pip install --upgrade pip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pip
upgrade still needs to happen before us, I think.
I'm inclined to merge #19396 ahead of this. It looks like we are currently failing on building numpy. The cause of the current failure is
which is due to a change in the internal cpython c-api's for handling generators interacting with code produced by cython. There is a fix to this in cython (cython/cython#3921) but not released yet. This seems a bit too far from our core-concerns to be worth testing? |
Merging #19396 sounds 👍 to me |
Possibly fixes #19352. I think it might be better to try installing matplotlib first anyway, so if that fails there hasn't been a load of wasted time installing the test dependencies. Maybe I've missed something though.