Skip to content

fix(pylock): merge a partial environment over defaults in select() - #1437

Open
yunaremaia wants to merge 2 commits into
pypa:mainfrom
yunaremaia:fix-pylock-select-partial-environment
Open

yunaremaia wants to merge 2 commits into
pypa:mainfrom
yunaremaia:fix-pylock-select-partial-environment

Conversation

@yunaremaia

Copy link
Copy Markdown

Summary

Pylock.select() reads python_full_version straight out of its environment argument:

# src/packaging/pylock.py
env_python_full_version = _pep440_python_full_version(
    environment["python_full_version"]
    if environment
    else default_environment()["python_full_version"]
)

Nine lines earlier, the same parameter is built as dict(environment or {}, ...) # Marker.evaluate will fill-up — it carries overrides merged over default_environment(), which is exactly what Marker.evaluate() does (markers.py: current_environment |= environment). So the override semantics held for packages.marker but not for requires-python.

An empty mapping is falsy and accidentally took the default branch, which is why environment={} worked. Any other partial mapping raised an unhandled KeyError out of a generator whose documented failure mode is PylockSelectError:

>>> list(lock.select(environment={"sys_platform": "linux"}))
KeyError: 'python_full_version'
>>> Marker('sys_platform == "linux"').evaluate({"sys_platform": "linux"})  # same dict
True

Changes

  • Read python_full_version from the merged default, so a partial mapping behaves as documented instead of raising KeyError.
  • Widen the annotation to Environment | Mapping[str, str | AbstractSet[str]] | None, matching both what select() now accepts and Marker.evaluate()'s signature, and document the override semantics in the docstring.
  • Refuse a set-valued python_full_version with PylockSelectError instead of letting it reach the version parser. This mirrors the existing isinstance guard in markers.py for set-valued environment keys.

Tests

tests/test_pylock_select_environment.py — 9 tests: four partial mappings select as if merged, an empty mapping still behaves like the default, a full Environment still selects, a real python_full_version override is honoured (and still fails requires-python when it should), an unsatisfiable lock still raises PylockSelectError on a partial environment, and the set-valued case is refused.

Verified against main at 4a920d6:

  • New tests fail before the change (KeyError), pass after.
  • Full suite: 62,455 passed, no regressions (62,446 pre-existing + 9 new).
  • ruff check, ruff format --check and mypy clean on both touched files.

The change is a no-op for callers that already pass a complete Environment or {}, so it is safe to release in a patch release.

`select()` reads `python_full_version` straight out of the `environment`
argument:

    env_python_full_version = _pep440_python_full_version(
        environment["python_full_version"]
        if environment
        else default_environment()["python_full_version"]
    )

Nine lines earlier the same parameter is built as `dict(environment or {},
...)  # Marker.evaluate will fill-up`, i.e. it carries *overrides* merged
over `default_environment()` -- which is exactly what `Marker.evaluate()`
does (`current_environment |= environment`). So the override semantics
held for `packages.marker` but not for `requires-python`.

An empty mapping is falsy and accidentally took the default branch, so
`environment={}` worked. Any other partial mapping raised an unhandled
`KeyError` out of a generator whose documented failure mode is
`PylockSelectError`:

    list(lock.select(environment={"sys_platform": "linux"}))
    KeyError: 'python_full_version'

Read the key from the merged default instead, and widen the annotation to
`Environment | Mapping[str, str | AbstractSet[str]] | None` so the
parameter's declared type matches both what `select()` accepts and what
`Marker.evaluate()` accepts. A set-valued `python_full_version` is a
caller error and is now refused with `PylockSelectError` rather than
reaching the version parser.
@henryiii

henryiii commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Added a review below, looks like you can get mismatched python_version / python_full_version's. Deriving python_version from python_full_version seems fine, and the other way should probably not be allowed. Pinging @sbidoul for a proper review, but this might be an easy thing to fix up first.

🤖 AI text below 🤖

The background /code-review finished. It found one issue.

src/packaging/pylock.py:712 (medium): the requires-python checks and the marker checks can use different Python versions when environment is partial.

If the caller gives python_version but not python_full_version, python_full_version comes from the host interpreter. Marker.evaluate uses the overridden python_version. The per-package requires-python check (near line 762) has the same problem.

Two examples, with the host on Python 3.12:

  • The lock has requires-python = ">=3.13" and a package marker python_version >= '3.13'. select(environment={"python_version": "3.13"}) raises PylockSelectError, but the markers treat the environment as 3.13.
  • The target is {"python_version": "3.8"} and the lock needs >=3.10. The check passes against the host's 3.12 with no error, so it selects packages for an interpreter that the lock does not support.

Before this change, these calls raised KeyError. Now they give a wrong result with no error, and the docstring says passing only the keys that differ is enough. The new test case test_partial_environment_is_merged_over_defaults[{"python_version": "3.14"}] passes only because of this mismatch.

Possible fixes:

  • Reject python_version when python_full_version is not also given.
  • Derive the missing key from the other one.
  • Document that both keys must be given together.

The review found no problems in the rest of the change. The empty and full mappings behave as before. A set-valued python_full_version is refused cleanly. The wider type annotation matches Marker.evaluate. All 9 new tests pass.

@sbidoul

sbidoul commented Oct 6, 2026

Copy link
Copy Markdown
Member

This changes the method signature to accept a partial environment. Is this desirable?

The two spellings of the interpreter were read from different places.
Marker.evaluate merges environment over default_environment(), so the
markers saw the override, while the requires-python checks subscripted
environment["python_full_version"] and fell back to the host. Passing
only python_version therefore checked the override against the version of
the interpreter running select().

Resolve the pair once, on the mapping handed to the markers, and read
every check from it. python_full_version pins the interpreter, so it wins
and the major.minor spelling is derived from it. python_version alone does
not pin the patch level that requires-python is checked against, so it is
refused instead of being guessed at.

Signed-off-by: Yunare Maia <yunare@gmail.com>
@yunaremaia

yunaremaia commented Oct 7, 2026 •

Copy link
Copy Markdown
Author

Thanks both — the review was right, and this was a real defect in the PR rather than a nit.

The problem. The two spellings of the interpreter were read from different places. Marker.evaluate merges environment over default_environment(), so the markers saw the override; the requires-python checks subscripted environment["python_full_version"] and fell back to the host. Passing only python_version therefore checked the override against the version of the interpreter running select() — which is exactly the mismatch you flagged.

Applied in 6ac49f4:

  • Resolve the pair once, on the same env mapping handed to the markers, and read every check from it. pylock.environments was also evaluating against the raw environment instead of that mapping, so it now uses env too — otherwise a third consumer could disagree with the other two.
  • python_full_version pins the interpreter, so it wins when both are passed and the major.minor spelling is derived from it. That matches "deriving python_version from python_full_version seems fine".
  • python_version alone is now refused with a PylockSelectError, per "the other way should probably not be allowed". It does not pin the patch level that requires-python is checked against, so the two options were to guess one or reject the call; guessing means checking a version nobody asked for, which is the same class of bug. The docstring says so.
  • The set-valued guard now covers python_version as well as python_full_version.

On the signature question (@sbidoul): a partial mapping is accepted deliberately. select() documents the contract itself — dict(environment or {}, ...) # Marker.evaluate will fill-up — and Marker.evaluate takes a partial mapping for the same purpose. Accepting one here means the two agree; rejecting it would leave select() stricter than the helper it delegates to. If you would rather have a named Environment-only signature and reject partial mappings outright, that is a small change and I am happy to make it — but the KeyError it raised on main should not come back either way.

Tests — tests/test_pylock_select_environment.py, now 15 cases:

  • Four new cases cover the mismatch: an override the host cannot satisfy still selects; the derived major.minor follows python_full_version; python_version alone is refused; pylock.environments sees the same version as the marker checks.
  • The parametrised partial-mapping case that used {"python_version": "3.14"} was replaced — it asserted the behaviour that is now an error.

Verified on 6ac49f4:

  • New tests fail before the change and pass after. Reverting only src/packaging/pylock.py leaves 4 failing and the 11 pre-existing cases passing on both sides, so the new ones are not asserting something incidental.
  • tests/test_pylock.py, tests/test_pylock_select.py, tests/test_pylock_select_environment.py and tests/test_markers.py: 2418 passed.
  • ruff check and ruff format --check clean on both touched files.
  • The full suite is not run here (VPS memory budget); CI is the verification.

CI status: the five workflows (Test, Linting, Documentation, CodeQL, Performance Benchmarks) have sat at action_required with zero jobs since the PR opened on 2026-10-03 — there is no check run at all on either head. That is GitHub's approval gate for a first-time contributor's fork, not a failure, so the local runs above are currently the only verification. One approval on the workflows will let the real suite run.

@sbidoul

sbidoul commented Oct 7, 2026

Copy link
Copy Markdown
Member

My point is that Environment can't be partial, so the example of the OP violates the API contract.

If you have use cases for pylock.select to accept a partial environment I'd suggest you open an issue to describe them in your own words so we can discuss if an API change is worthwhile.

My current position is that it's not, since it's easy for the caller to create a full environment and update it as desired.

So I'm -1 on this PR as it stands.

@notatallshaw

Copy link
Copy Markdown
Member

btw, I have a separate change that achieves the same goals with a different approach, I'll open an issue first.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants