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

failing builds on python 3.7 #4256

Closed
ltalirz opened this issue Jul 11, 2020 · 9 comments
Closed

failing builds on python 3.7 #4256

ltalirz opened this issue Jul 11, 2020 · 9 comments

Comments

@ltalirz
Copy link
Member

ltalirz commented Jul 11, 2020

We already reported in a couple of places that installing AiiDA in the latest Github action runners with python 3.7.8 fails actions/runner-images#1202

The reason is that, for some reason, the downgrade from pyyaml 5.3.2 to pyyaml 5.1.2 fails in that environment.

While we've temporarily switched to the python 3.8 environments for the CI tests that run on every commit, the nightly test install runs are still failing.

There are a couple of options for working around this issue:

  1. keep using python 3.7.8 but uninstall pyyaml before installing aiida-core
        pip uninstall -y pyyaml
  1. use the setup-python action v2 and fix the version of the python environment to 3.7.7
  2. Upgrade pyyaml; see also this comment

Since 1) is a bit of a hack, and this issue isn't really our fault, I would suggest 2).
3) might still take a bit of time since this update apparently first has to happen in plympy.

@csadorf @sphuber @chrisjsewell Sounds good?

@sphuber
Copy link
Contributor

sphuber commented Jul 12, 2020

I would say 1 is just as much a hack as number 2. I was going to propose to just switch from Python 3.7 to 3.8 entirely, as we did for the CI workflow, but then I saw that we actually run the test_install.yml for all Python versions, so there we would anyway have to find a solution for 3.7.

I still have some questions though:

#. You say the problem is with uninstalling pyyaml on 3.7.8, but then you propose doing exactly that as a fix. Does it really work if we just uninstall before running aiida-core? So somehow the problem only arises when pip tries to uninstall it as part of a bigger install? Seems weird.
#. Do we know if this is just a problem with 3.7.8 and pyyaml==5.1.2 on the GHA runner, or can we expect the same to happen on an actual machine that has Python 3.7.8. If so, having just a workaound defeats the purpose of having an automated test that checks the software can be installed on all Python versions.

Finally, just wanted to mention that 3 might be even more work than one would think. It is not just plumpy that will have to be updated, but kiwipy as well. However, doing so would be relatively simple, if it weren't for the fact that upgrading will break the checkpointing of processes. This is the very reason I pinned the version in the first place. It is not exactly clear what the implications are, but we have to be a bit careful.

@ltalirz
Copy link
Member Author

ltalirz commented Jul 12, 2020

You say the problem is with uninstalling pyyaml on 3.7.8, but then you propose doing exactly that as a fix. Does it really work if we just uninstall before running aiida-core? So somehow the problem only arises when pip tries to uninstall it as part of a bigger install? Seems weird.

Correct. I did test this on github actions - uninstalling pyyaml works; installing pyyaml 5.1.2 afterwards works; only directly installing 5.1.2 pyyaml fails.
See the discussion & GH actions links on actions/runner-images#1202

Do we know if this is just a problem with 3.7.8 and pyyaml==5.1.2 on the GHA runner, or can we expect the same to happen on an actual machine that has Python 3.7.8.

That is indeed something left to do - I wanted to test this through conda, but python 3.7.8 was not yet available.
If someone else is willing to test things (first install pyyaml 5.3.2, then install pyyaml 5.1.2) that would be great.

If so, having just a workaound defeats the purpose of having an automated test that checks the software can be installed on all Python versions.

It doesn't defeat the purpose - pyyaml is not part of the standard library and won't be installed in a new environment by default.
The github actions virtual environment happens to have it installed, which forces us to work around it.

Anyhow, figuring out whether this is a problem of the github actions runner environment or whether it is a problem of pyyaml would be worthwhile (in the first case, we should reopen actions/runner-images#1202 )

@sphuber
Copy link
Contributor

sphuber commented Jul 12, 2020

It doesn't defeat the purpose - pyyaml is not part of the standard library and won't be installed in a new environment by default.
The github actions virtual environment happens to have it installed, which forces us to work around it.

What if a user tries to install aiida-core in an environment that already has the newer version of pyyaml installed. I agree that is going to be unlikely, but that is what I meant with whether it could be a "real" problem and not just of our CI

@csadorf csadorf self-assigned this Jul 13, 2020
@csadorf
Copy link
Contributor

csadorf commented Jul 13, 2020

I tried to install aiida-core on the official Python docker container and that seems to work without any issue:

docker run python:3.7.8 /bin/bash -c "python --version && git clone https://github.com/aiidateam/aiida-core.git && cd aiida-core && python -m pip install -e . && python -m pip freeze"

Similarly, when install pyyaml first and then install aiida-core, no problem:

docker run python:3.7.8 /bin/bash -c "python --version && python -m pip install pyyaml && python -m pip freeze && git clone https://github.com/aiidateam/aiida-core.git && cd aiida-core && python -m pip install -e . && python -m pip freeze"

This leads me to conclude that that this is primarily an issue within the CI environment, but we can of course not rule out that this problem occurs in some user environments as well.

@csadorf
Copy link
Contributor

csadorf commented Jul 14, 2020

@sphuber Based on my tests I would like to proceed with option 2, any objections?

@sphuber
Copy link
Contributor

sphuber commented Jul 14, 2020

Fine for me to move ahead with option 2

@csadorf
Copy link
Contributor

csadorf commented Jul 15, 2020

Since builds are no longer failing, do we consider this issue closed?

@sphuber
Copy link
Contributor

sphuber commented Jul 15, 2020

I kept it open when I merged the PR because you didn't mark it as fixing this issue. I thought this was intentional because you called it a "stepping stone" to actually solving the issue. Which is probably by relaxing the pyyaml requirement. But I think there might be another issue for this. If this is true, I am happy to close this.

@csadorf
Copy link
Contributor

csadorf commented Jul 15, 2020

I was unsure myself whether we should consider this closed.

I've added the topics/dependencies/constraint issue label to keep track of issues that are causing a particular constraint and applied it to #3709 for pyyaml. Since we are keeping track of this there, I think it is fair to say that this issue here is resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants