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

Docs: Add release version pinning to install instructions #4147

Merged
merged 2 commits into from
Jun 5, 2020

Conversation

chrisjsewell
Copy link
Member

Conda is having a hard time solving the environment without versions specified (I think due to some tricky dependency requirement balances between aiida-core and erlang). It basically hangs on:

$ conda create -n aiida -c conda-forge aiida-core aiida-core.services
Collecting package metadata (current_repodata.json): done
Solving environment: failed with repodata from current_repodata.json, will retry with next repodata source.
Collecting package metadata (repodata.json): done
Solving environment: - 

Providing the actual version appears to solve this, and so in this PR I have added the release version to the install instructions.
Following https://stackoverflow.com/a/27418207/5033292, the release version is a variable, and so will auto-update for new releases of aiida-core.

@chrisjsewell chrisjsewell requested a review from csadorf June 4, 2020 04:18
@chrisjsewell
Copy link
Member Author

@codecov
Copy link

codecov bot commented Jun 4, 2020

Codecov Report

Merging #4147 into develop will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4147      +/-   ##
===========================================
+ Coverage    78.86%   78.87%   +0.01%     
===========================================
  Files          467      467              
  Lines        34481    34481              
===========================================
+ Hits         27189    27192       +3     
+ Misses        7292     7289       -3     
Flag Coverage Δ
#django 70.79% <ø> (+0.01%) ⬆️
#sqlalchemy 71.66% <ø> (+0.01%) ⬆️
Impacted Files Coverage Δ
aiida/transports/plugins/local.py 80.47% <0.00%> (+0.26%) ⬆️
aiida/engine/daemon/client.py 73.72% <0.00%> (+1.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f112cd9...9948337. Read the comment docs.

Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

That's really annoying, but if that's the only viable work-around then we have no choice. Is there a related issue on the conda source code or so that we could subscribe to?

Is the change also necessary for pip?

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Jun 4, 2020

if that's the only viable work-around then we have no choice. Is there a related issue on the conda source code or so that we could subscribe to?

I think it is. It's hard to tell exactly, because it just stalls at "Solving environment". But I imagine it is mainly related to this issue: conda-forge/aiida-core-feedstock#30, which is "fixed" on our end (well now that conda-forge/aiida-core-feedstock#32 is also closed). But a full fix I guess may be to make the rabbitmq feedstock go back and pin all their old versions to erlang version ranges AND mark the old builds as broken, then maybe Conda would not get so confused.

Is the change also necessary for pip?

No, I just did this for consistency, but can change it back?

@chrisjsewell
Copy link
Member Author

Having a bit more of a think about this:
Probably a root issue here is that aiida-core and aiida-core.services are two completely separate packages, when really you want services to act like extra requirements do in setup.py, e.g. aiida-core[services].
Having a quick google, in the long term (this fix will still be required for now) it might be good to add a https://docs.conda.io/projects/conda-build/en/latest/resources/define-metadata.html#run-constrained in the aiida-core package, to tie it to the same version of aiida-core.services i.e.:

requirements:
  run_constrained:
    - aiida-core.services =={{ version }}

which would constrain the possibilities for conda environment solving

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Jun 4, 2020

BTW we were discussing conda/pip interoperability during the docs_revamp, and I just come across this option: https://docs.conda.io/projects/conda/en/latest/user-guide/configuration/pip-interoperability.html
and there is also this post that maybe could be linked to in the installation documentation: https://www.anaconda.com/blog/using-pip-in-a-conda-environment

@csadorf
Copy link
Contributor

csadorf commented Jun 4, 2020

if that's the only viable work-around then we have no choice. Is there a related issue on the conda source code or so that we could subscribe to?

I think it is. It's hard to tell exactly, because it just stalls at "Solving environment". But I imagine it is mainly related to this issue: conda-forge/aiida-core-feedstock#30, which is "fixed" on our end (well now that conda-forge/aiida-core-feedstock#32 is also closed). But a full fix I guess may be to make the rabbitmq feedstock go back and pin all their old versions to erlang version ranges AND mark the old builds as broken, then maybe Conda would not get so confused.

Should we put this PR a bit on hold to see if conda-forge/aiida-core-feedstock#33 potentially resolves this?

Is the change also necessary for pip?

No, I just did this for consistency, but can change it back?

I am not sure, I guess if we recommend that for conda, then we should also do it for pip. One issue that I see is that installing with conda install foo and conda install foo=x.y is really not the same, because conda remembers that you made a specific version requirement and will therefore not automatically update (that might be a good thing). Using conda install foo==x.y is even more restrictive.

I think this is less of an issue with pip, where installing foo=x.y will not put any restrictions on future updates.

@csadorf
Copy link
Contributor

csadorf commented Jun 4, 2020

BTW we were discussing conda/pip interoperability during the docs_revamp, and I just come across this option: https://docs.conda.io/projects/conda/en/latest/user-guide/configuration/pip-interoperability.html
and there is also this post that maybe could be linked to in the installation documentation: https://www.anaconda.com/blog/using-pip-in-a-conda-environment

Very interesting. I think we should test that and potentially have this reflected in our docs once the beta testing phase has ended and in case this works well.

@sphuber
Copy link
Contributor

sphuber commented Jun 4, 2020

I am not sure, I guess if we recommend that for conda, then we should also do it for pip.

I would actually argue that we should only put this for conda, as that is the exception. Calling without explicit version number for pip should always get the latest version, which is what we want.

@chrisjsewell
Copy link
Member Author

Ok well I'll close conda-forge/aiida-core-feedstock#33 today, and see if this helps first.

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Jun 5, 2020

Yeh mehh; with the changes, on my workstation it still takes ~100 seconds to solve the environment with no version guidance (~10 with pinning), which is hardly ideal for a "quick-start".
(note if you set the python version it only takes ~15, i.e. conda create -n aiida python=3.7 aiida-core aiida-core.services).

I have removed the pinning for pip

@chrisjsewell chrisjsewell requested a review from csadorf June 5, 2020 13:28
@sphuber
Copy link
Contributor

sphuber commented Jun 5, 2020

Fine for me to merge. When we do, though, please add the reason very clearly in the commit, as stated by @chrisjsewell that it is for performance reasons

@csadorf csadorf merged commit a0e39c6 into aiidateam:develop Jun 5, 2020
@chrisjsewell chrisjsewell deleted the conda-install branch January 27, 2021 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants