Skip to content

Select pre-release collections that are inevitable #81764

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

Open
wants to merge 3 commits into
base: devel
Choose a base branch
from

Conversation

webknjaz
Copy link
Member

Previously, the resolvelib dependency resolver that we use for managing the collection installations, was set up to disregard pre-releases met in the dependnecy tree unless --pre was passed on the CLI by the end-user or the collection didn't have any stable releases published to Galaxy or an Authomation Hub instance.

With this patch, our dependency resolver will start considering pre-releases that are the only way to satisfy requirements met in the dependnecy tree. Such as, when a dependency is requested restricted by a version range that doesn't match any stable releases but can be satisfied by pre-releases in that range. Though, such pre-releases would be disregarded if there are pre-installed collections that already satisfy the requested requirements.

As with many of our previous design considerations, this one is inspired by the Python packaging ecosystem, pip and PEP 440. It is intended to align better with the natural end-user expectations, from the UX perspective.

SUMMARY

$sbj.

ISSUE TYPE
  • Bugfix Pull Request
ADDITIONAL INFORMATION

N/A

@webknjaz webknjaz force-pushed the bugfixes/inevitable-pre-releases-in-collection-dependency-tree branch from fada668 to 7274714 Compare September 22, 2023 17:02
@ansibot ansibot added bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. labels Sep 22, 2023
Copy link
Contributor

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

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

Do you have an example of pip working like this? I tried with a package I knew had a pre-release and wasn't able to see it use the pre-release. For example pip install 'pypsrp>=0.9.0' will not install the 1.0.0b1 release. Maybe it's because it's a beta of a major version bump but it would be good to see evidence that this is how Pip works.

To me it doesn't make too much sense to silently bring in a pre-release, it should be explicitly requested either by a pre-release requirement somewhere or with --pre in the cli args.

@webknjaz
Copy link
Member Author

webknjaz commented Sep 26, 2023

@jborean93

Do you have an example of pip working like this?

It doesn't and is declared a bug worth fixing which I've additionally confirmed with a Pip maintainer in a private chat: pypa/pip#11888 (comment). I'm going to make sure it's fixed in pip.

To me it doesn't make too much sense to silently bring in a pre-release, it should be explicitly requested either by a pre-release requirement somewhere or with --pre in the cli args.

https://peps.python.org/pep-0440/#handling-of-pre-releases says it should work like this. And I agree — offering an obvious solution is better then erroring out. Also, --pre is for globally allowing pre-releases anywhere in the tree but the behavior this PR introduces allows just enough of pre-releases to fill in the gaps in the tree, preferring stable versions everywhere else.

It'd be weird if a transitive dependency can only be satisfied with pre-releases so it'd require --pre effectively forcing the end-user to allow unstable versions for all of their other unrelated dependencies.

But I agree that maybe it's worth printing out a warning when there's no --pre and the resolver selects a pre-release.

I talked to @s-hertel on Slack and we agreed that this is how the resolver should behave. And not just because we've drawn a lot of inspiration from Pip and PEP 440 — it's just more intuitive. And I think this is the right thing to do.

@jborean93
Copy link
Contributor

jborean93 commented Sep 26, 2023

I don't wish to stand in the way of this, I just think that using a pre-release without an explicit opt in acknowledgement is not ideal. This is because pre-releases can contain half implemented or potentially bug prone code and IMO a user should be explicitly acknowledging they are fine with this either through an explicit requirement somewhere or some flag/setting.

https://peps.python.org/pep-0440/#handling-of-pre-releases says it should work like this.

While it does, it places it under the section

Dependency resolution tools SHOULD also allow users to request the following alternative behaviours:

This to me says that it should allow this behaviour but not necessarily as the default, just a way to opt into it. The first example here seems to be covered by --pre while this proposed PR is trying to solve the second scenario. But like --pre maybe this should be covered under a different argument, or --pre is expanded to take in a value to define the --pre behaviour chosen. It also says it could be an error rather than something that will be resolved normally.

Personally I would wait to merge this behaviour until pip actually includes it rather than just it being the desired behaviour. The PEP is worded in a way that this seems to be opt-in behaviour and even then the ultimately action is still up to the implementation.

@s-hertel
Copy link
Contributor

s-hertel commented Sep 26, 2023

I talked to @s-hertel on Slack and we agreed that this is how the resolver should behave. And not just because we've drawn a lot of inspiration from Pip and PEP 440 — it's just more intuitive. And I think this is the right thing to do.

To be clear, I haven't looked at this in any depth. But we are in agreement that the current behavior does not match the PEP 440 (for multiple reasons).

@webknjaz
Copy link
Member Author

@jborean93 I'd rather not wait for pip since there's no timeline yet. I think I'd add a warning/error that would depend on a CLI argument but I'm convinced that letting the resolve return something over nothing is a more pleasant UX. Ultimately, the end-users are still responsible for getting their deps in order but in case of the transitive deps, they're dependent on the packagers' decisions and can't do anything about that. And if those packagers depend on pre-release ranges, they probably know what they're doing so I'd argue it's an explicit pre-release request. We still should make it obvious to the end-users, of course.

@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Sep 26, 2023
@webknjaz
Copy link
Member Author

But I agree that maybe it's worth printing out a warning when there's no --pre and the resolver selects a pre-release.

@jborean93 I implemented this in aaa04eb.

Erroring out pending.

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Sep 27, 2023
@webknjaz
Copy link
Member Author

webknjaz commented Sep 28, 2023

Erroring out pending.

@jborean93 this is implemented in 38f1ccc 648a74b via --forbid-inevitable-pre-releases CLI flag, making the PR ready for review.

@webknjaz webknjaz requested a review from jborean93 September 28, 2023 18:11
@webknjaz webknjaz force-pushed the bugfixes/inevitable-pre-releases-in-collection-dependency-tree branch from 38f1ccc to 648a74b Compare September 28, 2023 21:39
@jborean93
Copy link
Contributor

Ultimately I'm a +0 on this. I can see that this being compliant with PEP 440 but I don't think that people should be creating collections that depend on a pre-release version without them explicitly putting in the pre-release requirement. Having a version flag without a non-prerelease available seems like a package smell to me.

@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Oct 2, 2023
@webknjaz webknjaz requested a review from sivel October 2, 2023 20:58
@webknjaz

This comment was marked as outdated.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Oct 16, 2023
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label May 21, 2024
@ansibot ansibot added the stale_pr This PR has not been pushed to for more than one year. label Jan 28, 2025
@webknjaz webknjaz force-pushed the bugfixes/inevitable-pre-releases-in-collection-dependency-tree branch from 648a74b to cd11ecd Compare February 5, 2025 02:07
@ansibot ansibot removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_pr This PR has not been pushed to for more than one year. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Feb 5, 2025
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 19, 2025
Previously, the resolvelib dependency resolver that we use for
managing the collection installations, was set up to disregard
pre-releases met in the dependnecy tree unless `--pre` was passed
on the CLI by the end-user or the collection didn't have any stable
releases published to Galaxy or an Authomation Hub instance.

With this patch, our dependency resolver will start considering
pre-releases that are the only way to satisfy requirements met in
the dependnecy tree. Such as, when a dependency is requested
restricted by a version range that doesn't match any stable releases
but can be satisfied by pre-releases in that range. Though, such
pre-releases would be disregarded if there are pre-installed
collections that already satisfy the requested requirements.

As with many of our previous design considerations, this one is
inspired by the Python packaging ecosystem, pip and PEP 440. It is
intended to align better with the natural end-user expectations, from
the UX perspective.
This warning is only shown when the end-user doesn't explicitly
request pre-release versions to be considered globally but the
dependency resolver returns some despite that.
Prior to this patch, whenever the dependency resolver resorted to
falling back to selecting pre-release collection candidates to satisfy
the dependency tree requirements, the CLI program would print out a
warning, listing such pre-release collections that ended up being a
part of the dependency tree.

This change allows the end-users to specify
`--forbid-inevitable-pre-releases` command-line option that will turn
the warning into an error. The pre-release candidates are still not
being rejected early during the resolution but are returned in the
tree, making it possible to display the list of pre-releases that are
the only way to satisfy the dependency requirements.
@webknjaz webknjaz force-pushed the bugfixes/inevitable-pre-releases-in-collection-dependency-tree branch from cd11ecd to 8411c31 Compare March 11, 2025 16:12
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Mar 11, 2025
@webknjaz webknjaz moved this from 📑 Ready 👌 to 🗒️ Backlog 📝 in 📅 Procrastinating in public Mar 12, 2025
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Mar 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants