Skip to content

Pre release v2.12.0rc1#83

Merged
sloosvel merged 15 commits intoconda-forge:rcfrom
sloosvel:v2.12.0rc1
Feb 14, 2025
Merged

Pre release v2.12.0rc1#83
sloosvel merged 15 commits intoconda-forge:rcfrom
sloosvel:v2.12.0rc1

Conversation

@sloosvel
Copy link
Copy Markdown
Contributor

@sloosvel sloosvel commented Feb 13, 2025

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.

@conda-forge-admin
Copy link
Copy Markdown
Contributor

conda-forge-admin commented Feb 13, 2025

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/meta.yaml) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe/meta.yaml:

  • ℹ️ noarch: python recipes should usually follow the syntax in our documentation for specifying the Python version.
    • For the host section of recipe, you should usually use python {{ python_min }} for the python entry.
    • For the test.requires section of recipe, you should usually use python {{ python_min }} for the python entry.
    • If the package requires a newer Python version than the currently supported minimum version on conda-forge, you can override the python_min variable by adding a Jinja2 set statement at the top of your recipe (or using an equivalent context variable for v1 recipes).

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13330963972. Examine the logs at this URL for more detail.

@sloosvel
Copy link
Copy Markdown
Contributor Author

I would need some help to address the remaining suggestions

@valeriupredoi
Copy link
Copy Markdown
Contributor

for the new suggestions use the main recipe, @bouweandela and I have put those in there https://github.com/conda-forge/esmvalcore-feedstock/blob/main/recipe/meta.yaml

let me have a looksee why the build tests fail

Copy link
Copy Markdown
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

env looks fine otherwise; the build halts on iris and we did plug Numpy 2 support

@valeriupredoi
Copy link
Copy Markdown
Contributor

BTW do not pin to python<3.13 (as is in the 2.11.1 recipe) since we now have support for 3.13, maybe <3.14 or just leave blank the upper limit 🍺

sloosvel and others added 2 commits February 14, 2025 08:56
Co-authored-by: Valeriu Predoi <valeriu.predoi@gmail.com>
@sloosvel
Copy link
Copy Markdown
Contributor Author

for the new suggestions use the main recipe, @bouweandela and I have put those in there https://github.com/conda-forge/esmvalcore-feedstock/blob/main/recipe/meta.yaml

Thanks, I think I followed the scheme in b66c599 . I also set no upper limit.

I still get a message about the {{ python_min }} definition though. Do I need to do anything else?

@valeriupredoi
Copy link
Copy Markdown
Contributor

It needs that silly python min set in the linux64 yml file too, sorry, forgot to tell you about it, see #81

@sloosvel
Copy link
Copy Markdown
Contributor Author

It needs that silly python min set in the linux64 yml file too, sorry, forgot to tell you about it, see #81

Thanks, added. Now it looks like the docker build is failing though...

@valeriupredoi
Copy link
Copy Markdown
Contributor

I'll have a look in a second

@valeriupredoi
Copy link
Copy Markdown
Contributor

valeriupredoi commented Feb 14, 2025

types-package_resources seems to have been yanked https://pypi.org/project/types-pkg-resources/ - they say use types-setuptools instead 🤷‍♂️

@sloosvel
Copy link
Copy Markdown
Contributor Author

Ok at least now it reaches the tests, two of them are failing:

FAILED tests/integration/preprocessor/_io/test_load.py::TestLoad::test_load_grib - OSError: One or more of the files specified did not exist:
    * "$PREFIX/lib/python3.13/site-packages/tests/sample_data/iris-sample-data/polar_stereo.grib2" didn't match any files
FAILED tests/unit/preprocessor/test_shared.py::test_apply_mask[mask2-array2-dim_map2-expected2] - AssertionError: 
Arrays are not equal

They seem to run fine on main though?

===== 4025 passed, 15 skipped, 4 xfailed, 852 warnings in 86.58s (0:01:26) =====

@valeriupredoi
Copy link
Copy Markdown
Contributor

one sec, am looking at this, it appears we do need iris-sample-data in our conda env, let me just confirm that

@valeriupredoi
Copy link
Copy Markdown
Contributor

a few suggestions, I compared the latest stable with this branch:

  • use pip install ESMValTool-sample-data types-requests types-PyYAML so not even the types_setuptools at all
  • remove the obsolete and commented out # - types-pkg_resources
  • move - python >={{ python_min }} at the top of the run dependencies (this is just aesthetic ie for consistency with the main recipe)

@valeriupredoi
Copy link
Copy Markdown
Contributor

oh and add - python {{ python_min }} to test: requires: group too so the tests are run with min Python as per conda forge's new requirements (which is BS by me, but heyho). If you give me write access to your branch then I can do these changes, Saskia

@sloosvel
Copy link
Copy Markdown
Contributor Author

Done! You should also be able to commit to the branch since maintainers are allowed to do edits.

@valeriupredoi
Copy link
Copy Markdown
Contributor

Done! You should also be able to commit to the branch since maintainers are allowed to do edits.

ah that's a good point, forgot about that. OK, let's see what's the hiccup this time around...

@valeriupredoi
Copy link
Copy Markdown
Contributor

valeriupredoi commented Feb 14, 2025

there's something very weird with the pip install, look how it declares the dependencies:

Requirement already satisfied: cartopy>=0.21 in $PREFIX/lib/python3.13/site-packages (from scitools-iris>=2.2->ESMValTool-sample-data) (0.24.0)

this should be from the ESMValTool dep tree, not anything to do with iris nor ESMValTool-sample-data

Am looking into it right now, @bouweandela have you seen this oddity before? I've not

EDIT: nevermind, am an idiot - that's coming from pip installing ESMValTool-sample-data - should be fine 😮‍💨

@valeriupredoi
Copy link
Copy Markdown
Contributor

OK I know what's going on in here:

  • we need tests/sample_data/iris-sample-data/polar_stereo.grib2 because that's in our main but somehow got omitted from the release package (not iris' fault, as I initially thought)
  • we also need a certain PR that got merged that changed the chunking mechanism, let me dig that one out

@sloosvel what branch did you use for the rc1 release?

@sloosvel
Copy link
Copy Markdown
Contributor Author

@sloosvel what branch did you use for the rc1 release?

I used branch v2.12.x, the code freeze was done on Tuesday 11th so everything that got merged before then should be included

@valeriupredoi
Copy link
Copy Markdown
Contributor

valeriupredoi commented Feb 14, 2025

good work, Saskia! I checked all the contents of the rc1 PyPI package - all there as it's supposed to be. OK, one error at a time: the missing grib file fail is because conda-build is looking in the wrong location:

file_specs = ['/home/conda/feedstock_root/build_artifacts/esmvalcore_1739532511799/_test_env_placehold_placehold_placehold_placehol...lacehold_placehold_placehold_place/lib/python3.13/site-packages/tests/sample_data/iris-sample-data/polar_stereo.grib2']

it should have ESMValCore in there not straight up in top level tests that clearly don't exist - this is because the test itself says:

    def test_load_grib(self):
        """Test loading a grib file."""
        grib_path = Path(
            Path(esmvalcore.__file__).parents[1],
            "tests",
            "sample_data",
            "iris-sample-data",
            "polar_stereo.grib2",
        )
        cubes = load(grib_path)

I suspect that makes the path jump one dir up - @schlunma you added GRIBs in Core, could you pls have a look at this one for me? 🍺

@valeriupredoi
Copy link
Copy Markdown
Contributor

the failed mask test comes from @bouweandela ESMValGroup/ESMValCore@d0bfb58#diff-573168b58f1cccbdfdc802bd9c7fd26883d42c4eb463b55aaf845bccfae47e95 in ESMValGroup/ESMValCore#2520 - we need to understand why the test fails in this configuration here, am looking at it

@valeriupredoi
Copy link
Copy Markdown
Contributor

the mask test fail is 99% because conda build uses dask==2025.2.0-pyhd8ed1ab_0 - let me try this in our env, if this is the case, then it'll make yet another absolutely unwanted and annoying bug coming from Dask

@valeriupredoi
Copy link
Copy Markdown
Contributor

boom! Son of a *** bloody Dask it is - these guys will run us into the ground with all their massive unplanned, undocumented changes they make on a whim! OK we need that pin <2025.2 not just for that test but also heck knows what else the new Dask breaks. The only failed test is that GRIB file one - I propose we just skip it for the RC1 and ask nicely @schlunma to fix it for the stable release?

@sloosvel
Copy link
Copy Markdown
Contributor Author

Many thanks V! I guess the environment file will have to be updated as well?

I propose we just skip it for the RC1 and ask nicely @schlunma to fix it for the stable release?

Sounds good to me, but as you prefer, you are the reviewer. Should I then go on with the checklist and re-render?

@valeriupredoi
Copy link
Copy Markdown
Contributor

Many thanks V! I guess the environment file will have to be updated as well?

Indeed, I opened ESMValGroup/ESMValCore#2663 where we can fix all the bugs with 2025.2, in the meantime we should open a PR pin dask as in here, in the package.

I am all for skipping the load test, we know it works well from tests in upstream, but we ought to fix it before the main release. @bouweandela @schlunma do you folks agree with this?

@bouweandela
Copy link
Copy Markdown
Contributor

I am all for skipping the load test, we know it works well from tests in upstream, but we ought to fix it before the main release. @bouweandela @schlunma do you folks agree with this?

Yes, that should be fine for this release candidate.

Indeed, I opened ESMValGroup/ESMValCore#2663 where we can fix all the bugs with 2025.2, in the meantime we should open a PR pin dask as in here, in the package.

With Dask at 2025.1 as the only remaining option, it may be beneficial to relax the pin on Dask in ESMValCore to allow for versions older than 2024.8. What if someone discovers a bug in 2025.1 too?

@valeriupredoi
Copy link
Copy Markdown
Contributor

thanks @bouweandela - I agree about Dask - let's do the release testing with 2025.1 as it is now, since this one looks to be the least problematic, and it's modern too. I plugged in the test ignore in cbd8d23 - this should allow the package build with no fails 🍺

Copy link
Copy Markdown
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

excellent work @sloosvel pls go ahead and rerender, we can push this wee one now 🍺

@sloosvel
Copy link
Copy Markdown
Contributor Author

@conda-forge-admin, please rerender

@sloosvel sloosvel merged commit 8968792 into conda-forge:rc Feb 14, 2025
4 checks passed
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