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

Disable automerge, enable Grayskull checking and fix PyPI URL #15

Merged
merged 4 commits into from
Nov 8, 2024

Conversation

rgommers
Copy link
Contributor

@rgommers rgommers commented Nov 8, 2024

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 gh-9
Closes gh-11
Follows up on gh-13

conda-forgegh-12 contained a breaking change due to a dependency version bump,
and that wasn't caught by the linter. Having a reviewer look at PRs
is better.
@rgommers rgommers requested a review from asmeurer as a code owner November 8, 2024 10:13
@conda-forge-admin
Copy link
Contributor

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 almost always follow the syntax in our documentation. For the host section of the recipe, you should almost always use python {{ python_min }} for the python entry. For the run section of the recipe, you should almost always use python >={{ python_min }} for the python entry. For the test.requires section of the recipe, you should almost always use python {{ python_min }} for the python entry. You may need to override the python_min variable in the conda_build_config.yaml/variants.yaml if the package requires a newer Python version than the currently supported minimum version on conda-forge.

@jakirkham
Copy link
Member

jakirkham commented Nov 8, 2024

Thanks Ralf! 🙏

The linter error would be fixed by following the recently added CFEP 25

Once done we could add a "fixes" note for issue: #10

Edit: Added a "fixes" note for issue ( #11 ), which you already fixed in this PR

@rgommers
Copy link
Contributor Author

rgommers commented Nov 8, 2024

The linter error would be fixed by following the recently added CFEP 25

I looked at the link, but I don't like that at all. Adding {{ python_min }} and then defining python_min in a separate file is just obscure indirection for no real reason as far as I can tell. It's very hard to understand such constructs unless you're intimately familiar with conda-forge (it's not like the variable is imported so it's obvious where it comes from).

@rgommers
Copy link
Contributor Author

rgommers commented Nov 8, 2024

The rationale in the CFEP makes sense for excluding too-old versions no longer supported by conda-forge, but it doesn't discuss the case where a package only supports a newer version well enough.

Something like a double constraint >={{ python_min }}, >=3.10 could make sense.

@rgommers
Copy link
Contributor Author

rgommers commented Nov 8, 2024

I couldn't even easily find what the current value of python_min was - it took time to locate it:

https://github.com/conda-forge/conda-forge-pinning-feedstock/blob/a15fa36d14319f4045acefbff2dd3a55ed454f80/recipe/conda_build_config.yaml#L796-L799

This really should be easier to introspect for the average feedstock maintainer.

@jakirkham
Copy link
Member

Thanks Ralf! 🙏

This is good feedback. Would it be possible to consolidate this feedback in issue ( conda-forge/conda-forge.github.io#2351 )? That would really help us with tracking it

As this is a recent CFEP, fully expect there is room for improvement

@rgommers
Copy link
Contributor Author

rgommers commented Nov 8, 2024

Thanks, I'll do that! Will try to add some suggestions about what would have helped me.

@asmeurer
Copy link
Member

asmeurer commented Nov 8, 2024

I don't think we should disable automerge.

@rgommers
Copy link
Contributor Author

rgommers commented Nov 8, 2024

I don't think we should disable automerge.

Why not? It's pretty trivial to review and merge a PR, just a quick look at changelog, lints, etc. Automerge is apparently dangerous. I think even aside from the review opportunity, one of the advantages of a delay of 1-2 days is that regressions do not affect conda-forge users. This is actually quite valuable - PyPI users can report the obvious regressions.

@asmeurer
Copy link
Member

asmeurer commented Nov 8, 2024

It's an extra step, and one that I will probably forget to do when making a release, especially given that the bot tends to have a delay.

@asmeurer
Copy link
Member

asmeurer commented Nov 8, 2024

Going to merge this and leave the automerge off for now because I want to make sure the next release is correct. But I do think we should turn it back on in the future.

@asmeurer asmeurer merged commit 17a2197 into conda-forge:main Nov 8, 2024
4 checks passed
@jakirkham
Copy link
Member

It's an extra step, and one that I will probably forget to do when making a release, especially given that the bot tends to have a delay.

It's possible to request a bot update, which will pick up the new version and now dependencies

Does this project have a release process checklist? If so, maybe this can be an item there

@asmeurer
Copy link
Member

asmeurer commented Nov 8, 2024

Can that be done automatically in the GitHub Actions job that does the release?

@jakirkham
Copy link
Member

Can that GHA job raise an issue on a different repo?

@asmeurer
Copy link
Member

asmeurer commented Nov 9, 2024

Yeah, that's the question. I have no idea if that's possible. I'm assuming not. I guess we could try adding gh issue create to the job and seeing if it works.

@jakirkham
Copy link
Member

Yeah it's a question of how to configure the permissions correctly

If you find a solution that works for you, would be interested to hear about it. Would imagine this is useful in more places

Ofc the other angle would be teaching the bot to subscribe to webhooks: regro/cf-scripts#18

@rgommers rgommers deleted the no-automerge branch November 9, 2024 12:54
@rgommers
Copy link
Contributor Author

rgommers commented Nov 16, 2024

Will try to add some suggestions about what would have helped me.

I wanted to do this this morning, and then noticed I'm not the only one with this feedback. The docs now explicitly address the "lower version is newer" case and there is now a way to define python_min inside meta.yaml (see docs). So I think I'm happy with the current state now - will incorporate python_min in a future PR.

@jakirkham
Copy link
Member

Glad to hear that Ralf! 🙏

If you think of more feedback, please let us know 🙂

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.

PyPI source/url should replace pypi.io with pypi.org Consider using Grayskull
4 participants