Ignore dependencies not matching the project's python constraint...

[radoering]

...no matter if the python version is specified by "python" keyword or by "markers"

Pull Request Check List

Relates-to: #4952

• Added tests for changed code.
• Updated documentation for changed code.

Currently, the following equivalent dependency specifications result in different lock files:

[tool.poetry.dependencies]
python = "~3.8"
pandas = {version="1.3.*", python="~3.9"}

[tool.poetry.dependencies]
python = "~3.8"
pandas = {version="1.3.*", markers="python_version=='3.9.*'"}

When using python the pandas dependency not matching the project's python constraint is ignored. When using markers the dependency is not ignored.

This PR harmonizes the behaviour between both variants.

[maciejskorski]

@radoering
I believe this is related to my issue #4959.
Glad to see that pandas example I shared are that successful for debugging :-)

Questions:

1. Is the solution you propose to early-prefilter, similar as in my PR [optimize] early-ignore dependencies not matching chosen markers #4956?
2. I believe the issue with constraints goes bit deeper - the keyword python and and pep508 markers seem to be not quite compatible?

[radoering]

@maciejskorski

1. This PR only fixes inconsistent behavior and doesn't change the fact that the lock file is independent from the current environment (platform and Python version). The Python version of the used environment does not matter here. Only the Python constraint in the pyproject.toml is relevant. It may be similar but it is not the same. A filter on python_version in your PR filters dependencies by the current Python version (which is exactly one version). Here the dependencies are filtered by all Python versions matching the Python constraint of the pyproject.toml.
2. Probably, you are right. But that has to be solved in another PR. ;)

[maciejskorski]

@radoering

My two cents

1. Thanks for clarification! ;)

2. I see you proposed to inherit the missing constraint from the parent, is that right?
   As documented in detail in poetry doesn't parse python constraints when markers are present #4965, the real issue is that the keyword-type and marker-type python constraints exclude each other, as per the current code logic. Furthermore, it seems we already have this filtering mechanism in place

   poetry/src/poetry/puzzle/provider.py

   Lines 494 to 496 in 1aa4d5b

   	for dep in requires:
   	if not self._python_constraint.allows_any(dep.python_constraint):
   	continue

   it is just broken by bugs in the parser. By fixing the parser (to collect versions as expected and promised in docs) we would fix the problem at the root for good, which to me sounds better than this post-hoc patch?

[radoering]

2. I see you proposed to inherit the missing constraint from the parent, is that right?

The constraint is propagated from the root package to the root dependency. This makes the filter logic you reference take effect.

2. the real issue is that the keyword-type and marker-type python constraints exclude each other

The issue addressed by this PR has nothing to do with keyword-type and marker-type python constraints excluding each other. If you look at the examples at the top, you will see there is either a keyword-type or a marker-type constraint (not both together).

2. By fixing the parser (to collect versions as expected and promised in docs) we would fix the problem at the root for good, which to me sounds better than this post-hoc patch?

Fixing the root cause sounds good, but when I revert the fix in version_solver.py from this PR and apply the changes to factory.py from your PR, the test added in this PR fails. Thus, I suppose this PR is independent of your PR.

[maciejskorski]

@radoering

I run your example, and that's why I encourage you to consider the code I provided in reciprocity:-)

So, if you set the debugger at the guilty lines I indicated above

poetry/src/poetry/puzzle/provider.py

Lines 494 to 496 in 1aa4d5b

	for dep in requires:
	if not self._python_constraint.allows_any(dep.python_constraint):
	continue

[d._python_constraint for d in requires]

you will see that

• the filtering you propose is already in place
• the filtering in your use-case fails as python version is not populated at all -wrongly. (reason: poetry doesn't parse python constraints when markers are present #4965)

fix the root-cause rather than patching post-hoc, and we will be all fine?
I mean, doesn’t work for you to make Python constraint populate either from the keyword or from the marker?

As of now, yes, both our PRs are drafts and not fully tested.

[radoering]

@maciejskorski: I'm afraid I don't quite follow you.

I see that there are several places where filtering takes place. At first, the lines you mention (= early filtering). And later the following lines (= late filtering), which also were in place before this PR:

poetry/src/poetry/puzzle/provider.py

Lines 692 to 713 in 1aa4d5b

	for dep in dependencies:
	if not package.dependency.transitive_marker.without_extras().is_any():
	marker_intersection = (
	package.dependency.transitive_marker.without_extras().intersect(
	dep.marker.without_extras()
	)
	)
	if marker_intersection.is_empty():
	# The dependency is not needed, since the markers specified
	# for the current package selection are not compatible with
	# the markers for the current dependency, so we skip it
	continue
	dep.transitive_marker = marker_intersection
	if not package.dependency.python_constraint.is_any():
	python_constraint_intersection = dep.python_constraint.intersect(
	package.dependency.python_constraint
	)
	if python_constraint_intersection.is_empty():
	# This dependency is not needed under current python constraint.
	continue

The fix in this PR triggers one of the continue statements in these lines by setting python_versions of the root dependency.

I run your example, and that's why I encourage you to consider the code I provided in reciprocity:-)

I understand that filtering earlier is better. However, I am not sure which code you're referring to. If I don't mess something up, your first commit 6d5409b does neither trigger early nor late filtering and fails the test added in this PR. Your later commit ec42fa5 triggers early filtering and passes the test. However, other tests in test_solver.py fail when I apply ec42fa5. In contrast, the fix in this PR does not break any existing test.

Nevertheless, even if there is a solution that triggers early filtering for this use case and passes all tests, I don't see a reason why python_versions of the root dependency should not be set.

[maciejskorski]

@radoering

Sorry, I think we are both active on few issues and are about to mix topics so let me try to clarify better.

First, nothing logically wrong with picking the lost constraint from the parent/root.
However it seems the matching mechanism you propose to add, already exists.
For that referenced above provider .py lines 494-496. What do you see if you set the debugger there as I did?

If your matching mechanism, which I called filtering, already exists better to fix the bug, rather than repeating it once more.

[radoering]

@maciejskorski: OK, so I've wrongly assumed that you are referring to the other PR. Sorry about that.

If I set a breakpoint at the mentioned lines and execute the new test, I see that package A is discarded because a python_constraint is set and package B is not discarded because no python constraint is set. Thus, ensuring that a python_constraint is set in package B somehow would be an alternative solution.

However it seems the matching mechanism you propose to add, already exists.

I don't propose to add any matching mechanism. The filtering already exists. It is just not triggered because python_versions of the dependency is not set. As you can see in https://github.com/python-poetry/poetry/pull/4958/files, I only added one line of code in version_solver.py and a test.

[maciejskorski]

@radoering

Yes! We are closer and closer to agree on this. Like I detailed in related issues, the reason for non-setting constraints comes from the backend in poetry-core. Namely, the python constraint was not deducted from the marker. Instead a fix there, you propose a valid but somewhat add-hoc fix after the real issue happens.

I checked out that poetry-core in its recent version improves handling markers which seems to solves the issue

Please make sure you install poetry-core@master (not any precached version from pip). Then on my machine, the problem is resolved as python constraints and markers are aligned. Essential is to get enhanced Dependency.marker property.

https://github.com/python-poetry/poetry-core/blob/af08f1ce720da467c9bf3d43eed3d9ebaf4ad7fb/src/poetry/core/packages/dependency.py#L181

[radoering]

You are right. It seems the issue has already been fixed upstream. Thanks for pointing this out.

I still wonder if there are some cases where early filtering is not possible so that setting the root dependency's python constraint is required for late filtering. Maybe, I'll dig a bit deeper if I find some time.

[radoering]

Closing PR because issue has already been fixed upstream. Created separate PR # #5307 to add only the test case.

[github-actions]

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