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

add mini-piggyback migrator for removing pin_compatible("numpy"...) #2470

Merged
merged 3 commits into from
Apr 20, 2024

Conversation

h-vetinari
Copy link
Contributor

Fixes #2469; goes hand-in-hand with (the migrator filename used in) conda-forge/conda-forge-pinning-feedstock#5790

This is a baby version of #2135 / #1668, where we've already extensively tested the used functionality. Strictly speaking, we could even avoid the whole dance with _slice_into_output_sections and just directly remove any lines matching the regex pattern for pin_compatible("numpy",...) in the meta.yaml. However, I left it in for now in case anyone wants to add some logic that takes into account existing host-/run-dependencies, because then we need to operate on a per-section basis.

Copy link

codecov bot commented Apr 20, 2024

Codecov Report

Attention: Patch coverage is 45.45455% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 71.23%. Comparing base (3ecfc69) to head (ebcdb4f).

❗ Current head ebcdb4f differs from pull request most recent head e6d8f7c. Consider uploading reports for the commit e6d8f7c to get more accurate results

Files Patch % Lines
conda_forge_tick/migrators/numpy2.py 47.36% 10 Missing ⚠️
conda_forge_tick/auto_tick.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2470      +/-   ##
==========================================
- Coverage   71.28%   71.23%   -0.06%     
==========================================
  Files         100      101       +1     
  Lines       10118    10140      +22     
==========================================
+ Hits         7213     7223      +10     
- Misses       2905     2917      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

The _slice_into_output_sections can raise an error. This made sense for the stdlib work, but here we could iterate through the lines and do a straight replacement.

Also, the code will fail if people do unusual but valid things in yaml like an explicit list. I don't think we should handle this last case maybe?

@h-vetinari
Copy link
Contributor Author

Also, the code will fail if people do unusual but valid things in yaml like an explicit list. I don't think we should handle this last case maybe?

You mean like:

run: [{{ pin_compatible("numpy") }}, something_else]

?

The _slice_into_output_sections can raise an error. This made sense for the stdlib work, but here we could iterate through the lines and do a straight replacement.

Fine by me.

@beckermr
Copy link
Contributor

Yes I mean like this run: [{{ pin_compatible("numpy") }}, something_else].

@h-vetinari
Copy link
Contributor Author

h-vetinari commented Apr 20, 2024

Yes I mean like this run: [{{ pin_compatible("numpy") }}, something_else].

It's already handled. The regular expression doesn't care what comes before the first {, nor what comes after "numpy" (to avoid that something like , upper_bound=... breaks detection).

@beckermr
Copy link
Contributor

Ah but if _replacer ignores the whole line, then we delete the entire run section?

@h-vetinari
Copy link
Contributor Author

h-vetinari commented Apr 20, 2024

Ah but if _replacer ignores the whole line, then we delete the entire run section?

It doesn't ignore the line, it deletes it (if the thing to be inserted is empty; as our "" here). If that ends up creating an empty run-section that's not worth cleaning up automatically IMO - too many corner cases (comments, etc.). We also didn't do that for boost or stdlib either, e.g. they produced

     run:
-       - boost-cpp

or

     run_constrained:
-       - __osx >={{ MACOSX_DEPLOYMENT_TARGET|default("10.9") }}  # [osx and x86_64]

and that was fine?

@beckermr
Copy link
Contributor

Yep agreed! I was just checking that I understood the code.

@beckermr beckermr enabled auto-merge April 20, 2024 11:55
@beckermr beckermr merged commit f8f13fd into regro:master Apr 20, 2024
2 checks passed
@h-vetinari h-vetinari deleted the numpy2 branch April 20, 2024 12:35
@jakirkham
Copy link
Contributor

Thanks Axel and Matt! 🙏

Have we seen this piggyback migrator applied in any PRs? Would be interesting to see how it is doing

@h-vetinari
Copy link
Contributor Author

Have we seen this piggyback migrator applied in any PRs? Would be interesting to see how it is doing

It explicitly looks for the numpy2.yaml from conda-forge/conda-forge-pinning-feedstock#5790. As long as that PR is not merged, the piggyback won't ever be applied.

@jakirkham
Copy link
Contributor

Interesting am curious is there a particular reason to wait for NumPy 2 to make this change?

As noted in issue ( #2469 ), this change should be ok to make with the current NumPy pinning

Please let me know if this is more to the story

@h-vetinari
Copy link
Contributor Author

is there a particular reason to wait for NumPy 2 to make this change?

You're right that we could already remove it based on the existing 1.x infrastructure, but it's a numpy-related change - IMO it makes sense to roll it out together with the numpy migrator, rather than just attach it to random migrators or version updates.

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.

Piggyback migrator to drop {{ pin_compatible("numpy") }}
3 participants