Fix nondeterminism with environment markers #5239

[bakhtiary]

this is a continuation of the following PR:
#5279

The issue

What is the thing you want to fix? Is it associated with an issue on GitHub? Please mention it.

Always consider opening an issue first to describe your problem, so we can discuss what is the best way to amend it. Note that if you do not describe the goal of this change or link to a related issue, the maintainers may close the PR without further review.

If your pull request makes a non-insignificant change to Pipenv, such as the user interface or intended functionality, please file a PEEP.

https://github.com/pypa/pipenv/blob/master/peeps/PEEP-000.md

The fix

How does this pull request fix your problem? Did you consider any alternatives? Why is this the best solution, in your opinion?

The checklist

• Associated issue
• A news fragment in the news/ directory to describe this fix with the extension .bugfix.rst, .feature.rst, .behavior.rst, .doc.rst. .vendor.rst. or .trivial.rst (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

[bakhtiary]

@matteius @oz123 the PR #5279 has moved here,

[matteius]

Shouldn't this return constraints.keys(), locked_deps or whatever calls .keys() on the constraints dictionary?

[oz123]

It depends on how it used down the road I guess. If we are just iterating it, like a list then it is ok. Since, iterating is over keys only.

[bakhtiary]

@matteius You are right, it would be nicer if we convert this set to a list when we are returning it. This particular function is actually internal to this class, but the function get_metadata is public I will fix this ...

[cclauss]

list(constraints) is an list while constraints.keys() is an iterator.

[dqkqd]

I think if we want to keep all the packages sorted before putting them into pip's resolver. We can simply sort self._constraints instead.

pipenv/pipenv/utils/resolver.py

Lines 632 to 647 in b6d9ad6

	@property
	def constraints(self):
	if self._constraints is None:
	self._constraints = [
	install_req_from_parsed_requirement(
	c,
	isolated=self.pip_options.build_isolation,
	use_pep517=self.pip_options.use_pep517,
	user_supplied=True,
	)
	for c in self.parsed_constraints
	]
	# Only use default_constraints when installing dev-packages
	if self.dev:
	self._constraints += self.default_constraints
	return self._constraints

Like this.

self._constraints.sort(key=lambda ireq: ireq.name)

Those sorted self._constraints will be put directly into resolver.

pipenv/pipenv/utils/resolver.py

Lines 680 to 685 in b6d9ad6

	def resolve(self):
	self.constraints # For some reason it is important to evaluate constraints before resolver context
	with temp_environ(), self.get_resolver() as resolver:
	try:
	results = resolver.resolve(self.constraints, check_supported_wheels=False)
	except InstallationError as e:

[matteius]

I think the only thing to decide on left is if we sort the constraints -- I personally think we should sort them where @dqkqd points to the code, because sorting them seems to be the most consistent way we could present them to the resolver. I suspect there is no rhyme or reason to the order the constraints get added, so sorting also seems to make sense in that regards.

[bakhtiary]

I think the only thing to decide on left is if we sort the constraints -- I personally think we should sort them where @dqkqd points to the code, because sorting them seems to be the most consistent way we could present them to the resolver. I suspect there is no rhyme or reason to the order the constraints get added, so sorting also seems to make sense in that regards.

I do agree that sorting the constraints can help with stability. However it would be outside the scope of this PR which is to make sure that we get the same answer given the same input.

I'd be very happy to work on the constraint sorting problem in another PR. Maybe we should open an Issue to talk about the exact behavior, should we have a flag? should we do it in the pipenv.py or in resolve.py?

[bakhtiary]

@matteius There are some tests failing should I look into them or can we merge this as it is?

[matteius]

@bakhtiary These test failures are expected as possible for the time being until they can be figured out why they sometimes fail and usually on mac os:

FAILED tests/unit/test_utils.py::test_convert_deps_to_pip[deps0-requests] - A...
FAILED tests/unit/test_utils.py::test_convert_deps_to_pip[deps1-requests[socks]]

The windows failure was an issue to install before the test runner ran, so I kicked off the failed jobs again.

Let's take up the issue of sorting them constraints in a separate issue/PR then. I suspect we can merge this later today.

[dqkqd]

@bakhtiary, self._constraints is actually those packages which we are trying to reorder. So sorting those packages is the same as sorting self._constraints. The variable name seems confuse though.

[dqkqd]

The issue is not fixed on main branch. Maybe packages's order is changing somewhere.

   ~/Doc/workspace/pipenv    main !2 ?2 ❯ ./stress-lock.sh
attempt 1: greenlet markers="python_version >= '3' and (platform_machine == 'aarch64' or (platform_machine == 'ppc64le' or (platform_machine == 'x86_64' or (platform_machine == 'amd64' or (platform_machine == 'AMD64' or (platform_machine == 'win32' or platform_machine == 'WIN32'))))))"
attempt 2: greenlet markers="platform_python_implementation == 'CPython'"
attempt 3: greenlet markers="python_version >= '3' and platform_machine == 'aarch64' or (platform_machine == 'ppc64le' or (platform_machine == 'x86_64' or (platform_machine == 'amd64' or (platform_machine == 'AMD64' or (platform_machine == 'win32' or platform_machine == 'WIN32')))))"
attempt 4: greenlet markers="python_version >= '3' and platform_machine == 'aarch64' or (platform_machine == 'ppc64le' or (platform_machine == 'x86_64' or (platform_machine == 'amd64' or (platform_machine == 'AMD64' or (platform_machine == 'win32' or platform_machine == 'WIN32')))))"
attempt 5: greenlet markers="python_version >= '3' and platform_machine == 'aarch64' or (platform_machine == 'ppc64le' or (platform_machine == 'x86_64' or (platform_machine == 'amd64' or (platform_machine == 'AMD64' or (platform_machine == 'win32' or platform_machine == 'WIN32')))))"
attempt 6: greenlet markers="python_version >= '3' and platform_machine == 'aarch64' or (platform_machine == 'ppc64le' or (platform_machine == 'x86_64' or (platform_machine == 'amd64' or (platform_machine == 'AMD64' or (platform_machine == 'win32' or platform_machine == 'WIN32')))))"
attempt 7: greenlet markers="python_version >= '3' and platform_machine == 'aarch64' or (platform_machine == 'ppc64le' or (platform_machine == 'x86_64' or (platform_machine == 'amd64' or (platform_machine == 'AMD64' or (platform_machine == 'win32' or platform_machine == 'WIN32')))))"
attempt 8: greenlet markers="platform_python_implementation == 'CPython'"
attempt 9: greenlet markers="platform_python_implementation == 'CPython'"

[cclauss]

Why not a frozenset?

[cclauss]

.keys() is not required here.

[cclauss]

Why not a frozenset?

[dqkqd]

As I mention in #5299 (comment). I think this should be revert because it does not solve linked issue.