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

Pre release v2.5.0rc4 #33

Merged
merged 3 commits into from
Mar 3, 2022
Merged

Pre release v2.5.0rc4 #33

merged 3 commits into from
Mar 3, 2022

Conversation

schlunma
Copy link
Contributor

@schlunma schlunma commented Mar 3, 2022

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Closes #30

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@schlunma
Copy link
Contributor Author

schlunma commented Mar 3, 2022

@conda-forge-admin, please rerender

@schlunma
Copy link
Contributor Author

schlunma commented Mar 3, 2022

One question on this: I cannot find the dependency pillow in our environment.yml, setup.py or meta.yml file. Do we need it?

It is present in doc/requirements.txt, but in this files there are many other dependencies which are not listed here (e.g., autodocsum, docutils, etc.).

EDIT: Similarly, cartopy is also not present in our ESMValCore dependencies (not even in doc/requirements.txt).

@schlunma
Copy link
Contributor Author

schlunma commented Mar 3, 2022

Does anyone no why this test is failing? It ran fine on the nightly builds on CircleCI and GitHub actions, and also runs fine on my local machine and mistral...

@zklaus
Copy link

zklaus commented Mar 3, 2022

I do not. Let's see if it is a fluke. If not, I'll have a look with tmate.

@conda-forge-admin, please restart ci

@zklaus
Copy link

zklaus commented Mar 3, 2022

So... I still don't know what's going wrong. But I can say that in the ESMValTool build we simply ignore the corresponding test, see here. However, in that case, we don't even ship the documentation by excluding it from distribution in the MANIFEST.in file. I don't recall the reason for that right now; it may simply be the size with all the pictures in it. For the core, that wouldn't be an argument since all the doc folder comes in at just over 800KB, which is not significant compared to the total size of 20MB, or the 11MB of the NE masks.

I think you should add the --ignore="tests/unit/documentation/test_duplications_in_changelog.py" switch, perhaps even for the whole tests/unit/documentation folder; when the build lands here, this has been sufficiently checked already.

@schlunma
Copy link
Contributor Author

schlunma commented Mar 3, 2022

Sounds good, thanks Klaus!! What do you think about the pillow and cartopy dependencies?

@valeriupredoi
Copy link
Contributor

valeriupredoi commented Mar 3, 2022

just took a look too - I am fairly sure conda build is ignoring the doc dir, it finds all Python stuff that need building and puts them in export SRC_DIR=/home/conda/feedstock_root/build_artifacts/esmvalcore_1646296297036/test_tmp for testing, but omitting doc

@valeriupredoi
Copy link
Contributor

we got rid of pillow in Core no? So I'm guessing that was the only place we needed it, but no more, what's the problem with cartopy?

@schlunma
Copy link
Contributor Author

schlunma commented Mar 3, 2022

Not a problem with cartopy, but it's not explicitly listed in any of our dependencies for ESMValCore, so I'm wondering if we need it here.

@valeriupredoi
Copy link
Contributor

I think we can remove both - @zklaus please confirm 🍺

@zklaus
Copy link

zklaus commented Mar 3, 2022

The silver search (ag, you may prefer to use e/f/grep or ack) shows that we use PIL aka pillow in esmvalcore._provenance and cartopy in esmvalcore.preprocessor._mask. Both are direct dependencies and should be added as such.

@schlunma
Copy link
Contributor Author

schlunma commented Mar 3, 2022

Makes sense, but then it would be good to add them to our environment.yml and setup.py, too, right?

@valeriupredoi
Copy link
Contributor

OK on the doc failed test - it's now clear that conda build does not touch the docs and does not include them in the package ie see the contents of esmvalcore-2.5.0rc3-pyh39db41b_1.tar.bz2 - it packages just the esmvalcore and during the build process it grabs only the tests dir from outside it to run the tests, so we sould not include any unit/integration tests that need files from doc

@valeriupredoi
Copy link
Contributor

cheers, Klaus! - about pillow and cartopy - they are both direct dependency requirements for iris, see their requirements - they also pin them (alas, pillow is needed only for the test env in iris) so I think we should include them too if we import them directly, but we should see how to get around not importing them directly, hence getting us rid of them as direct deps

@schlunma
Copy link
Contributor Author

schlunma commented Mar 3, 2022

Great, thanks for your input. Can we merge this?

@zklaus
Copy link

zklaus commented Mar 3, 2022

cheers, Klaus! - about pillow and cartopy - they are both direct dependency requirements for iris, see their requirements - they also pin them (alas, pillow is needed only for the test env in iris)

Of course they are.

so I think we should include them too if we import them directly, but we should see how to get around not importing them directly, hence getting us rid of them as direct deps

That doesn't make sense to me. You are saying we shouldn't use libraries ourselves that our dependencies are also using? By that token we shouldn't use Dask, Numpy, Scipy, or Python itself for that matter.

@schlunma schlunma merged commit 8c549dc into conda-forge:rc Mar 3, 2022
@schlunma schlunma deleted the v2.5.0rc4 branch March 3, 2022 12:45
@valeriupredoi
Copy link
Contributor

That doesn't make sense to me. You are saying we shouldn't use libraries ourselves that our dependencies are also using? By that token we shouldn't use Dask, Numpy, Scipy, or Python itself for that matter.

hahah, no, of course not - I'm saying we should see how(if) we can get around not using these two since they're each used in one single place and that is pretty specialized too. But that's for another time, no need to start looking into this now 👍

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.

4 participants