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

migrate noarch: python to CFEP-25's new syntax #2351

Open
11 of 13 tasks
beckermr opened this issue Nov 3, 2024 · 19 comments
Open
11 of 13 tasks

migrate noarch: python to CFEP-25's new syntax #2351

beckermr opened this issue Nov 3, 2024 · 19 comments

Comments

@beckermr
Copy link
Member

beckermr commented Nov 3, 2024

Per CFEP-25, noarch: python packages should have the syntax

host:
  - python {{ python_min }}
run:
  - python >={{ python_min }}

test:
  requires:
    - python {{ python_min }}

This issue has TODO items for the migration:

@beckermr beckermr changed the title blah migrate noarch: python to CFEP-25's new syntax Nov 3, 2024
weiji14 added a commit to regro-cf-autotick-bot/litdata-feedstock that referenced this issue Nov 6, 2024
weiji14 added a commit to conda-forge/litdata-feedstock that referenced this issue Nov 6, 2024
* updated v0.2.30

* MNT: Re-rendered with conda-build 24.9.0, conda-smithy 3.44.0, and conda-forge-pinning 2024.11.05.21.36.17

* Rename pypi.io to pypi.org

* Set python_min following CFEP-25

Using minimum python_requires defined at https://github.com/Lightning-AI/litdata/blob/v0.2.30/setup.py#L70. See details of CFEP-25 at conda-forge/conda-forge.github.io#2351 and https://conda-forge.org/docs/maintainer/knowledge_base/#noarch-python

---------

Co-authored-by: Wei Ji <[email protected]>
@beenje
Copy link
Contributor

beenje commented Nov 6, 2024

I understand the rationale about testing against the minimum supported python version but I'm afraid to miss issues with new python version.
distutils was removed from python 3.12. PEP 594 removed several modules from the standard library in 3.13. This can easily break some older packages.

I have an example recently where the build of taurus-feedstock failed with python 3.13. Issue was linked to a dependency, pint, not supporting 3.13. I had to patch the repodata.
If the package had been tested against oldest supported python, I'd not have noticed.

Best would be to test against oldest and newest python version, but I don't think that's possible right now.

Is the testing on minimum supported versions a recommendation? Will it be up to the maintainer?
Or more enforced?

@beckermr
Copy link
Member Author

beckermr commented Nov 6, 2024

The cfep allows overrides of any of these things. So yes, feel free. There are new defaults, not rules that have to be followed.

@h-vetinari
Copy link
Member

Worth nothing that for v1 recipes, you'll be able to test both the lowest and the highest supported python version, because the script: test section can be repeated.

@sjperkins
Copy link

The linter fails to substitute python_min into the

host:
  - python {{ python_min }}
run:
  - python >={{ python_min }}

test:
  requires:
    - python {{ python_min }}

For example the linting complains the following in PR

with

Configuring conda
+ [[ -f /home/conda/feedstock_root/LICENSE.txt ]]
+ cp /home/conda/feedstock_root/LICENSE.txt /home/conda/recipe_root/recipe-scripts-license.txt
+ [[ 0 == 1 ]]
+ conda-build /home/conda/recipe_root -m /home/conda/feedstock_root/.ci_support/linux_64_.yaml --suppress-variables --clobber-file /home/conda/feedstock_root/.ci_support/clobber_linux_64_.yaml --extra-meta flow_run_id=azure_20241107.6.1 remote_url=*** sha=f63d15ead9e3a6962930f62dc4b6bd76b427d26a
WARNING: No numpy version specified in conda_build_config.yaml.  Falling back to default numpy value of 1.26
Adding in variants from internal_defaults
Adding in variants from /home/conda/feedstock_root/.ci_support/linux_64_.yaml
Traceback (most recent call last):
  File "/opt/conda/lib/python3.12/site-packages/conda_build/metadata.py", line 2003, in _get_contents
    rendered = template.render(environment=env)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/conda/lib/python3.12/site-packages/jinja2/environment.py", line 1304, in render
    self.environment.handle_exception()
  File "/opt/conda/lib/python3.12/site-packages/jinja2/environment.py", line 939, in handle_exception
    raise rewrite_traceback_stack(source=source)
  File "/home/conda/recipe_root/meta.yaml", line 19, in top-level template code
    - python {{ python_min }}
jinja2.exceptions.UndefinedError: 'python_min' is undefined

Full log here:

@beckermr
Copy link
Member Author

beckermr commented Nov 8, 2024

This is a conda-build error, not a linter error. It appears you need to rerender the feedstock.

@maresb
Copy link
Contributor

maresb commented Nov 8, 2024

Thanks so much @beckermr for all your work on this, it's a huge improvement!!!

I'm running up against a few hard edges though, and I thought it may be useful to share:

  • The linter sounds way to authoritative given that we're still working out some of the details. It would be nice if possible to explain that that these are experimental and optional lints for new functionality. (Also, I doubt it's obvious to most people that rerendering is necessary.)
  • In case of packages that don't support the global python_min we need to override it, but there's no longer an example in the knowledge base for doing this.

@beckermr
Copy link
Member Author

beckermr commented Nov 8, 2024

I made a few PRs to help with this. Comments welcome!

@maresb
Copy link
Contributor

maresb commented Nov 8, 2024

Sorry I'm a bit late to this discussion. I have a concern about:

Maintainers may override the minimum Python version python_min in recipe/conda_build_config.yaml or change the build configuration as needed to match the constraints of their specific package.

I think it's a really nice feature of the feedstocks that routine updates for new releases can all be accomplished purely within recipe/meta.yaml (and then possibly rerendering). Several of the packages I work with follow the scientific Python versioning strategy or similar, and routinely require updates to python_min which are usually strictly above the global pin.

My concern is that in order to automate these updates, e.g. using Grayskull, now meta.yaml will no longer be sufficient since we also have to routinely edit conda_build_config.yaml. I think that as a maintainer this will result in a lot more annoying work.

What would you think of endorsing a strategy of setting python_min as a Jinja2 template variable at the top like this? This would be way easier to maintain in terms of Grayskull or similar.

@beckermr
Copy link
Member Author

beckermr commented Nov 8, 2024

For grayskull updates, I think it simply injects the minimum by hand anyways, so this is a non-issue?

@beckermr
Copy link
Member Author

beckermr commented Nov 8, 2024

Also note that the hint literally says "or change the build configuration as needed to match the constraints of their specific package."

@maresb
Copy link
Contributor

maresb commented Nov 8, 2024

For grayskull updates, I think it simply injects the minimum by hand anyways, so this is a non-issue?

What I take away as the biggest innovations from CFEP-25 are:

  1. Maintaining a source of truth for the minimum Python version
  2. Testing on that minimum Python version.

Grayskull does inject information from requires-python into the python constraint, but it currently does neither 1 nor 2.

My point is simply that there's potential for confusion if the knowledge base says to update recipe/conda_build_config.yaml but then we use a different implementation in Grayskull. It would be nice if there was a uniformly recommended way to override it. So I'm wondering if there are any advantages to using recipe/conda_build_config.yaml over instead using a Jinja2 variable?

@beckermr
Copy link
Member Author

beckermr commented Nov 8, 2024

Ah I see what you mean. Does the jinja2 override actually work? We have python_min in the global pinnings too and I am unsure which item takes precedence when jinja2 evaluates the templates.

@beckermr
Copy link
Member Author

beckermr commented Nov 8, 2024

My general feeling is that carrying important state in jinja2 is not a best practice, but I do see the advantage of not having conda_build_config,yaml files around for noarch: python recipes from the point-of-view of parsimony.

@maresb
Copy link
Contributor

maresb commented Nov 8, 2024

Does the jinja2 override actually work?

Ya. I'm not so experienced with Jinja2, and I'm no big fan of it either, but this example (same that I linked above) demonstrates a successful Jinja2 override, where without the set it picks up the global pin of 3.9 and the test fails because 3.9 is unsupported.

My general feeling is that carrying important state in jinja2 is not a best practice, but I do see the advantage of not having conda_build_config,yaml files around for noarch: python recipes from the point-of-view of parsimony.

Yes, agreed. I haven't looked into it, but I wonder if rattler offers a better way? Even if so, we probably need to stick to such basic methods for now.

EDIT: To answer my own question about rattler, the answer is affirmative; they introduce a "context variables" mechanism (source):

no full Jinja2 support: no conditional or {% set ... support, only string interpolation; variables can be set in the toplevel "context" which is valid YAML

@beckermr
Copy link
Member Author

beckermr commented Nov 8, 2024

ok I am happy to change the docs and linter to suggest context and/or jinja2. We need to ensure that rattler and conda-build have the same precedence order with respect to variants versus context items.

@beckermr
Copy link
Member Author

beckermr commented Nov 8, 2024

I tested rattler-build overrides and they work fine. I did however find an odd feature where rattler build treats recipe pins like

requirements:
  run:
    - python 3.10

as exact equality (eg renders to a spec that is ==3.10) as opposed to what conda-build does (which is adding a .* or equivalently =3.10).

I opened an issue here: prefix-dev/rattler-build#1174

@matthewfeickert
Copy link
Member

matthewfeickert commented Nov 12, 2024

Several of the packages I work with follow the scientific Python versioning strategy or similar, and routinely require updates to python_min which are usually strictly above the global pin.

Indeed. I think that having python_min explicitly in the feedstock recipe is quite powerful as it also serves as a way to validate that you remembered to update the Python lower bound in the feedstock when the corresponding Python package updates the requires-python metadata (c.f. PR #2370). So you now have an automated way of avoiding situations like what PR conda-forge/conda-forge-repodata-patches-feedstock#901 patched. That's pretty great!

Thank you @beckermr for also adding PR #2365 as I agree with @maresb that it is very nice if all the metadata for the feedstock can exist just in recipe/meta.yaml. That being said, I'm noticing in conda-forge/pyhf-feedstock#29 that if feedstocks use the Jinja2 set statement for setting the value of {{ python_min }} the linter treats this as having a problem (conda-forge/pyhf-feedstock#29 (comment)) but the linter doesn't treat the python_min map in recipe/conda_build_config.yaml approach as having an issue (conda-forge/pyhf-feedstock#28) (and to be clear in advance I have not had the time to dig into why yet, so I'm only reporting without doing any helpful work).

The linter also only mentions the use of python_min for

If the package requires a newer Python version than the currently supported minimum version on conda-forge

but it also allows for supporting patch releases of packages where the last minor release still supported a Python version that has been dropped from the supported global pinning. I can understand maybe not wanting to super advertise that old Pythons can still be used for builds in an effort to get people moving towards supporting more modern Pythons overall, but I personally find that to be pretty useful information.

@beckermr
Copy link
Member Author

Ah darn. I know what the bug is. Will fix it.

@beckermr
Copy link
Member Author

conda-forge/conda-smithy#2132

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

No branches or pull requests

6 participants