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

Improve intersection and union of markers #5192

Closed
radoering opened this issue Feb 11, 2022 · 7 comments
Closed

Improve intersection and union of markers #5192

radoering opened this issue Feb 11, 2022 · 7 comments
Labels
area/core Related to the poetry-core library kind/enhancement Not a bug or feature, but improves usability or performance

Comments

@radoering
Copy link
Member

radoering commented Feb 11, 2022

The handling of markers offers great potential for improvement. Even if marker simplifications seem intuitive to a human being, the subject is more complex than it might seem at first glance. Thus, it's best to proceed in small steps.

This is a meta issue to keep track of these steps:

Rationale

When resolving dependencies, markers play an important role. Especially, when multiple-constraints-dependencies come into play, overrides with contradicting markers should be ignored to avoid unnecessary failures in dependency resolution and speed up the resolution. See #4695 for details. In order to detect contradicting markers, intersections of markers that cannot be fulfilled have to be simplified to EmptyMarker. Analoguous, unions of markers that are always fulfilled have to be simplified to AnyMarker. At the moment, markers are not always simplified enough so that poetry is not able to detect if a marker can never be fulfilled (empty marker) or is always fulfilled (any marker).

Apart from this use case, simplifying markers seems to be mainly a cosmetic issue when looking into poetry.lock or an exported requirements.txt. (When checking if a dependency has to be installed in the current environment, it does not matter if a marker has been simplified or not.)

@radoering radoering added area/core Related to the poetry-core library kind/enhancement Not a bug or feature, but improves usability or performance labels Feb 11, 2022
@dimbleby
Copy link
Contributor

I think there's a good win to be had by teaching poetry-core that "python_version" and "python_full_version" are really the same thing. It's relatively common in my experience to end up with a marker like python_full_version < "3.0.0" and python_version >= "3.6".

Conceptually I'd suggest: convert both into a python_full_version. Probably that would be a mechanically tedious change to make though, poetry-core leans in the direction of favouring python_version at the moment.

@dimbleby
Copy link
Contributor

@radoering
Copy link
Member Author

radoering commented Feb 13, 2022

Conceptually I'd suggest: convert both into a python_full_version. Probably that would be a mechanically tedious change to make though, poetry-core leans in the direction of favouring python_version at the moment.

From a technical perspective I agree this is the most obvious solution. From a user's point of view I would prefer python_version compared to python_full_version because it's shorter and simpler. Maybe, after converting both to python_full_version and doing simplifications, python_full_verison could be converted to python_version if the full version ends with .0? Maybe, I would even only want to have a conversion if this results into a simplification and otherwise keep the original marker name. However, I am not sure if such a sophisticated solution is worth the effort because in the end it is only cosmetic.

By the way, a similar but maybe more challenging case is sys_platform vs. platform_system. Often you end up with a marker like sys_platform == "win32" and platform_system != "Windows". In order to simplify these kind of markers, some kind of mapping is required. However, I am afraid it might be impossible to create a complete mapping and missing entries may result in issues during dependency resolution...

@dimbleby
Copy link
Contributor

As you've seen I had a crack at the version / full_version case and it seems to have turned out fairly straightforward, albeit with room for still further improvement.

I think I've probably run out of ideas and enthusiasm in this area for now; not to mention that I'm already beginning to risk merge conflicts between the various MRs that are currently queued up. Over to you if you have more that you want to do - and to the maintainers to get these merged!

@dimbleby
Copy link
Contributor

Time to call this one done?

@radoering
Copy link
Member Author

radoering commented Aug 28, 2022

Yeah, should be quite good now.

Closing since there haven't been much improvements/fixes anymore in the last time and I can't come up with a good solution for merging sys_platform and platform_system.

Copy link

github-actions bot commented Mar 1, 2024

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/core Related to the poetry-core library kind/enhancement Not a bug or feature, but improves usability or performance
Projects
None yet
Development

No branches or pull requests

2 participants