Skip to content
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

remove explicit numpy install #4229

Merged
merged 1 commit into from
Jul 8, 2020
Merged

Conversation

ltalirz
Copy link
Member

@ltalirz ltalirz commented Jul 8, 2020

seems no longer needed... and possibly broke CI

Note: Creating a draft PR for this since @sphuber mentioned that tests were passing fine on his fork (so better test that things actually work on the official repo as well).

@sphuber
Copy link
Contributor

sphuber commented Jul 8, 2020

Note that this was put in because pymatgen will installs numpy during its build and it ignores our version requirements in the setup.json. This means that always the latest version of numpy will be installed if we install pymatgen, which has broken our builds multiple times in the past. Even though it works now then, I would not remove it, because as soon as numpy releases another release candidate, it will most likely break our builds.

@ltalirz
Copy link
Member Author

ltalirz commented Jul 8, 2020

@sphuber It looks like the issue is with the python 3.7 environment.
I can make this the only change for the moment

For unknown reasons, the python3.7 environment used for the "verdi" and
"pre-commit" jobs was broken.
@ltalirz ltalirz requested a review from sphuber July 8, 2020 15:00
@ltalirz ltalirz marked this pull request as ready for review July 8, 2020 15:00
@sphuber
Copy link
Contributor

sphuber commented Jul 8, 2020

It looks like the issue is with the python 3.7 environment.

Do we know why? As in, are we sure that this is not a random error that occurs and this time we got lucky and the tests passed? Reason I am asking is that the build on my fork ran without problems, so it looks like a transient problem no?

@ltalirz
Copy link
Member Author

ltalirz commented Jul 8, 2020

It may be transient, but tests failed on my fork, too.
I don't know the reason but we need to merge at least the docs PR; i.e. it is either ignore the failure or switch to python 3.8 env for the moment.

@sphuber
Copy link
Contributor

sphuber commented Jul 8, 2020

It may be transient, but tests failed on my fork, too.

Yeah, but so the point is that even if we merge this, the next build may just fail again. Or am I missing something?

@ltalirz
Copy link
Member Author

ltalirz commented Jul 8, 2020

Well, of course it can start failing again (and I'm sure at some point in the future it will again ;-) )
But for the time being it's fixed.

@ltalirz
Copy link
Member Author

ltalirz commented Jul 8, 2020

I think at the moment it may be specific to the python 3.7 env (which might have taken some time to propagate to your fork - you can check whether it fails now when you restart the workflow on your fork)

@ltalirz
Copy link
Member Author

ltalirz commented Jul 8, 2020

We could open a bug report here: https://github.com/actions/setup-python/issues

@ltalirz
Copy link
Member Author

ltalirz commented Jul 8, 2020

There is an open issue mentioning issues with the installation of dependencies
actions/setup-python#105

The ubuntu-latest runner has python 3.8 installed but not python 3.7:
https://github.com/actions/virtual-environments/blob/master/images/linux/Ubuntu2004-README.md

I'll add a comment.

@sphuber
Copy link
Contributor

sphuber commented Jul 8, 2020

I think at the moment it may be specific to the python 3.7 env

What is that based on though? Is there anything you saw that hints at this? Or is it just a guess?

@sphuber
Copy link
Contributor

sphuber commented Jul 8, 2020

you can check whether it fails now when you restart the workflow on your fork)

Just reran it and the verdi job ran fine even with Python 3.7: https://github.com/sphuber/aiida_core/runs/850405963

@ltalirz
Copy link
Member Author

ltalirz commented Jul 8, 2020

Then I guess it is just "the python 3.7 environment for the runners that are being used on aiidateam"?
I'll also restart the jobs for my old PR just in case

@ltalirz
Copy link
Member Author

ltalirz commented Jul 8, 2020

As expected, after restarting the runs of the docs PR, they still fail.
I say we merge this now. We can figure out later what exactly caused this.

@ltalirz
Copy link
Member Author

ltalirz commented Jul 8, 2020

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Merging this even if we don't understand if or why this change would fix the problem

@sphuber sphuber merged commit 4dfc01f into aiidateam:develop Jul 8, 2020
@ltalirz
Copy link
Member Author

ltalirz commented Jul 8, 2020

Opened issue on the corresponding repo actions/runner-images#1202

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.

None yet

2 participants