Skip to content

Prioritize editables over matching direct URL requirements - #14101

Draft
ichard26 wants to merge 3 commits into
pypa:mainfrom
ichard26:feat/editable-satisfy-direct-url
Draft

Prioritize editables over matching direct URL requirements#14101
ichard26 wants to merge 3 commits into
pypa:mainfrom
ichard26:feat/editable-satisfy-direct-url

Conversation

@ichard26

@ichard26 ichard26 commented Jun 24, 2026

Copy link
Copy Markdown
Member

Combines PR #13888 with a further enhancement to let editables override matching direct URL requirements.

This is done by processing editables first so they enter the editable candidate cache first so when any matching direct URL requirements are encountered, they're satisfied by the pre-existing editable candidate. This explicitly depends on PR #13888.

There are two problems here though:

  • By reusing the editable cache candidate for non-editable requirements, pip won't even build metadata for the regular requirement. While IMO we should avoid redundant builds, with our current caching implementation, this means inconsistent direct URLs will be missed. For example pip install -e file:./packages/pkgb "aa @ file:./packages/pkgb" would normally fail since the second requirement's only candidate would be discarded due to aa not matching the pkgb name. On this PR, this error is not caught. FWIW, extras are still requested correctly.

  • We don't handle equivalent but non-exact URLs well. On main, the factory caches link candidates based on exact URL.

    • This means an equivalent direct URL won't reuse the editable candidate if the URLs aren't identical
    • We don't resolve relative paths consistently , thus something like pip install -e ./packages/pkgb "file:./packages/pkgb" will fail.1 One is normalized to an absolute file URL, but the other one remains a relative file URL so they aren't treated as equivalent.

For more details, see the xfail tests I've added.

We can fix the second issue by normalizing relative file URLs and reusing the equivalent_links() logic for keying the link cache, but that makes the first problem more common since equivalent (editable) URLs will be able to shadow each other.

I haven't yet looked at the first issue.

Given these issues, do we still want to go forward with this feature? @sbidoul @notatallshaw

Footnotes

  1. This also fails on main if both requirements are editable + if both requirements are NOT editable.

pradyunsg and others added 3 commits April 19, 2026 19:12
When a transitive dependency referenced a package by direct URL and the
same package was also requested as editable, the resolver saw two
distinct candidate types for the same link and raised
`ResolutionImpossible`.

An editable install subsumes a non-editable direct URL request to the
same location, so `_make_base_candidate_from_link` now returns the
cached `EditableCandidate` in that case.

Co-authored-by: Pradyun Gedam <pradyunsg@users.noreply.github.com>
This just makes it a bit nicer to debug the code.
We already have a test[^1] checking that -e pkg where pkg is constrained
to the same location works. Now since direct URL dependencies can
satisfy matching top-level editable requirements, it makes sense to let
matching top-level editable and regular requirements to be mixed, with
the editable requirement "winning".

[^1]: test_new_resolver_succeeds_on_matching_constraint_and_requirement
@ichard26

Copy link
Copy Markdown
Member Author

Oh, FWIW, pip install file:./packages/pkgb "aa @ file:./packages/pkgb" already works without error with pip 26.1.2. That's not good, but I guess this PR doesn't make this problem worse ¯\_(ツ)_/¯.

@ichard26

Copy link
Copy Markdown
Member Author

Also our relative direct URL support is not well defined in the first place, see #6658 and specifically #6658 (comment).

@ichard26

Copy link
Copy Markdown
Member Author

TBH, we should address #7589 at the same we're making adjustments here because I've found it exceptionally difficult to keep all of the complexity and subtle edge cases in my head. Eventually, we should also document what we support officially for pip (probably here), but at least mapping out what our code currently supports (and maybe what we want to support) would be a good first step at reigning in this mess.

@ichard26 ichard26 added the skip PR template check Silence the PR template check in CI label Jul 5, 2026
@ichard26 ichard26 added this to the 26.2 milestone Jul 6, 2026
@ichard26

Copy link
Copy Markdown
Member Author

@notatallshaw @sbidoul I'm still waiting for an opinion here to help decide design decisions FYI. I'd like to make some progress as to not hold up the legacy resolver removal indefinitely.

@sbidoul

sbidoul commented Jul 12, 2026

Copy link
Copy Markdown
Member

@ichard26 I think both limitations you mention are acceptable.

I any case, I would personally continue to recommend not using direct URLs in dependencies, and use direct URL constraints instead.

I'd like to check if this PR fixes the failing test of #13904. Will try to do this tonight but I won't mind if you cherry picks the commits here.

@sbidoul

sbidoul commented Jul 15, 2026

Copy link
Copy Markdown
Member

I'd like to check if this PR fixes the failing test of #13904. Will try to do this tonight but I won't mind if you cherry picks the commits here.

@ichard26 Of course I did not manage to do it. Will try again next weekend.

@ichard26

ichard26 commented Jul 15, 2026

Copy link
Copy Markdown
Member Author

@sbidoul I just tested it locally, this does fix the failing test in #13904. You do need to revert including editableness when checking if a constraint is satisfied by a candidate to avoid failing the tests added in this PR, however, which AFAIU is expected.

@ichard26
ichard26 force-pushed the feat/editable-satisfy-direct-url branch from 1e91e16 to a9ccfb1 Compare July 18, 2026 02:18
@ichard26

Copy link
Copy Markdown
Member Author

I've pushed a fix for the failing CI. This is ready for review unless we want to address the limitations in the same PR. FWIW, addressing limitation (2) seems straightforward as a follow up.

@sbidoul

sbidoul commented Jul 18, 2026

Copy link
Copy Markdown
Member

When this is merged, I'll finish #13904

@sbidoul

sbidoul commented Jul 18, 2026

Copy link
Copy Markdown
Member

Thanks for tackling this and also testing with #13904 @ichard26

@ichard26

ichard26 commented Jul 20, 2026

Copy link
Copy Markdown
Member Author

@notatallshaw I'd like your review before pressing the green button.

Also, I took a closer look at the link URL normalization issues and the biggest source is from relative direct URLs which are not officially supported by pip, see #6658 -- thus I don't consider a blocking concern. I've prototyped a fix for the other source of issues related to equivalent but non-equal URLs and it's small.

@notatallshaw

Copy link
Copy Markdown
Member

@notatallshaw I'd like your review before pressing the green button.

I took a look over the weekend, and I wasn't able to convince myself this was strictly correct, but also I wasn't able to propose any concrete suggestions.

I'm not -1 on merging, because it's probably a net positive.

But I don't think I'm going to have capacity to give a full approval as I'm traveling most of next week.

@ichard26

ichard26 commented Jul 20, 2026 via email

Copy link
Copy Markdown
Member Author

@notatallshaw

Copy link
Copy Markdown
Member

This implementation.

@ichard26

ichard26 commented Jul 21, 2026

Copy link
Copy Markdown
Member Author

@notatallshaw your spidey sense was correct. While I wasn't able to find a case where this PR would do the wrong thing, I did realize that #13904 is buggy and would not interact well with this PR as is.

@sbidoul with #13904 for pip install pip -c constraints.txt where pip is editably constrained to a pip checkout, an editable base candidate is indeed returned, but at some point, the permit_editable_wheels install requirement attribute is lost, so pip builds a regular wheel:

$ pip install pip -c constraints.txt --no-build-isolation
Obtaining pip from file:///home/ichard26/dev/oss/pip
  Checking if build backend supports build_editable ... done
  Preparing metadata (pyproject.toml) ... done
Building wheels for collected packages: pip
  Building wheel for pip (pyproject.toml) ... done
  Created wheel for pip: filename=pip-26.1.dev0-py3-none-any.whl size=1812672 sha256=15cc6ff67a36736b59e132127b4ed7407d371e71a1f403996f6ed216d2450c8d
  Stored in directory: /tmp/pip-ephem-wheel-cache-6b9ik78q/wheels/02/b2/7b/cd44cb77bc4424b9ddde1ee0abd178e67d530bdc69f13f7fde
Successfully built pip
Installing collected packages: pip
  Attempting uninstall: pip
    Found existing installation: pip 26.2.dev0
    Uninstalling pip-26.2.dev0:
      Successfully uninstalled pip-26.2.dev0
Successfully installed pip-26.1.dev0

Interestingly, our test harness still thinks pip is editably installed.

In addition, for editable requirements/constraints to take priority reliably, they need to processed early as to prefill the editable candidate cache (which will be then reused by any matching regular requirements). However, constraints are processed later in the candidate discovery logic, causing the same buggy behaviour as above to occur when a matching regular requirement is user-supplied.

To support #13904 well and make everything more robust, I decided to do away with the implicit dependency on ordering the editable requirements first, and instead opted to record the links for all editable requirements and constraints early in collect_root_requirements. An editable base candidate is returned if any matching link is encountered. Since editable requirements/constraints are always known at the start of resolution, this should be correct(?).

Since this was all quite tricky, I've decided to open a new PR combining this PR and #13904 -> #14195. It includes fixes to the aforementioned bugs and a few more tests. I still need to review/stress-test my changes further before it's ready for review though.

@notatallshaw

Copy link
Copy Markdown
Member

Thanks for indulging me, I might have a little time before I fly Wednesday evening to give all of this a more thorough look and give a more evidence based review.

@ichard26

ichard26 commented Jul 24, 2026

Copy link
Copy Markdown
Member Author

I'm going to punt this PR, and by consequence, PR #13904 to pip 26.3. The latter has proven to be tricky with subtle bugs, and while I haven't (yet?) found any in this PR or its successor, I'd need to invest significant time to review my code to be confident that the change is indeed sound. More practically, I'm not comfortable making nontrivial changes in the resolver without Damian's review.

However, I do intend to come back to PR #14195 after the release since it's connected to the legacy resolver removal effort.

@ichard26 ichard26 removed this from the 26.2 milestone Jul 24, 2026
@ichard26
ichard26 marked this pull request as draft July 25, 2026 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided skip PR template check Silence the PR template check in CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants