Skip to content

Conversation

@valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Nov 9, 2022

sister PR to ESMValGroup/ESMValTool#2913

Motivation

Readthedocs builds use pip install --upgrade --upgrade-strategy eager to install and eventually reinstall everything from the env via pip - this is discussed in readthedocs/readthedocs.org#8890 and is not something good for many reasons, one of them, and a very recent and pertinent one is that the build will chuck a wobbly if the package versions differ from conda-forge to pypi.

Method

Use jobs with key names, here post env creation job

Docs output

https://esmvaltool--1786.org.readthedocs.build/projects/ESMValCore/en/1786/

Bears at a dinner party

I had to add all deps from setup.py to the conda-forge env file eventually dumping the good bits of #1542 into here so eventually closes #1541

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.

@valeriupredoi valeriupredoi added documentation Improvements or additions to documentation enhancement New feature or request labels Nov 9, 2022
@codecov
Copy link

codecov bot commented Nov 9, 2022

Codecov Report

Merging #1786 (1403dfb) into main (113bbc3) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1786   +/-   ##
=======================================
  Coverage   91.51%   91.51%           
=======================================
  Files         202      202           
  Lines       10908    10908           
=======================================
  Hits         9982     9982           
  Misses        926      926           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@valeriupredoi
Copy link
Contributor Author

blast! we need extensions in the main conda env - let me do that

@valeriupredoi valeriupredoi changed the title suppress installing and reinstalling deps with pip at readthedocs builds suppress installing and reinstalling deps with pip at readthedocs builds and add deps for docs build in conda environment Nov 10, 2022
@schlunma
Copy link
Contributor

Yeah, we should really think about merging #1542 soon

@valeriupredoi
Copy link
Contributor Author

Yeah, we should really think about merging #1542 soon

actually, I dunnit here 😁

@valeriupredoi valeriupredoi changed the title suppress installing and reinstalling deps with pip at readthedocs builds and add deps for docs build in conda environment Add all deps to the conda-forge environment and suppress installing and reinstalling deps with pip at readthedocs builds Nov 10, 2022
@bouweandela
Copy link
Member

Is this really ready for review @valeriupredoi? It looks like something you're still working on. Could you please put it in draft until you're happy with the results?

@valeriupredoi
Copy link
Contributor Author

@bouweandela this is ready for review - the reason all tests are failing is due to the new mypy and our fixing with either pinning mypy or your work in #1769 should go first in main, then we merge main here so to see everything 🟢

Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on @valeriupredoi! Would you mind using the pull request checklist to make the review a bit easier?

@valeriupredoi
Copy link
Contributor Author

how to get depressed on a Monday: look at this PR: Python 3.8 test fails with types crappe (again), Python 3.9 fails coz it cant find sample data, docs fail coz some deprecation warning - might as well go home now 🤣

@valeriupredoi
Copy link
Contributor Author

@bouweandela have a look at this - there are issues with pyesgf from conda-forge, it seems the two versions - c-f and pypi differ, with the c-f one throwing wobblies

@valeriupredoi
Copy link
Contributor Author

@bouweandela have a look at this - there are issues with pyesgf from conda-forge, it seems the two versions - c-f and pypi differ, with the c-f one throwing wobblies

I replicated test fails locally, it is not a matter of peculiar env from the GA test, but an actual problem with the conda-forge esgf-pyclient package, as opposed to the PyPI one that works very well. I will move esgf-pyclient to be downloaded only from PyPI for now, and will open an issue upstream at esgf-pyclient conda feedstock

@valeriupredoi
Copy link
Contributor Author

Here is the issue conda-forge/esgf-pyclient-feedstock#18

@valeriupredoi
Copy link
Contributor Author

@bouweandela I need to revert to pip install . without the nodeps since esgf is needed by the docs 😮‍💨

@valeriupredoi
Copy link
Contributor Author

wohoo! Finally, all tests are 🟢

@valeriupredoi
Copy link
Contributor Author

So I opened this conda-forge/esgf-pyclient-feedstock#19 to fix conda-forge/esgf-pyclient for older Pythons - not sure how long that PR will have to wait for approval, I say we merge this now, then we (I) will do a PR to re-add esgf-pyclient to our conda-forge deps and re-add the --no-deps to the Readthedocs pip install, when that PR will be accepted and merged 👍

@bouweandela
Copy link
Member

Apparently, I have permission to merge that, so this doesn't need to take long. Could you have a look at my comment on the pull request?

@valeriupredoi
Copy link
Contributor Author

yes you have, you bossman there too 😁 I did, and I operated the change, thanks, bud 🍺 Let's wait til that sees open ocean, then I can bring this up to standards

@bouweandela
Copy link
Member

Just merged it

@valeriupredoi
Copy link
Contributor Author

thanks a lot, bud! But am off to shove a 🍕 in the oven now, will fix this here tomorrow 👍

environment.yml Outdated
- iris>=3.2.1
- isodate
- jinja2
- mypy>=0.990
Copy link
Member

Choose a reason for hiding this comment

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

Could you move mypy to the test dependencies, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on it now, will also re add esgf-pyclient to the env file, and reinstate nodeps, fingers crossed this is done after, been geeting quite a few more white hairs on it 😁

@valeriupredoi
Copy link
Contributor Author

@bouweandela it's alive! It works well and tests are 🟢

@schlunma schlunma added the installation Installation problem label Nov 16, 2022
@schlunma schlunma merged commit 89fbe93 into main Nov 16, 2022
@schlunma schlunma deleted the suppress_eager_pip_readthedocs branch November 16, 2022 13:39
@schlunma
Copy link
Contributor

Awesome, thanks 🍻

@valeriupredoi
Copy link
Contributor Author

great! Many thanks to monsieur reviewer @bouweandela and monsieur le merger @schlunma - we are now all conda forged 💯

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

Labels

documentation Improvements or additions to documentation enhancement New feature or request installation Installation problem

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Install all dependecies from conda-forge

4 participants