poetry
poetry copied to clipboard
Don't search secondary repositories if not required
Not sure whether this is controversial or just obviously sensible...
Always searching secondary sources is bad for performance: and generally just isn't what people mostly expect.
Here's a behaviour change that skips secondary sources altogether, for packages that were provided by primary sources.
It's possible that this was the intended behaviour all along - the testcase test_solver_does_not_choose_from_secondary_repository_by_default
suggests as much. I'm not sure where the code that makes that test pass is! But anyway this is surely a more efficient way of achieving that goal.
this would fix eg #5959, #5096
If I'm not wrong it is controversial. What if two sources provide different versions of the same package? That's a common use case.
What if two sources provide different versions of the same package?
Per the new testcase: if the primary can meet the requirement, then only versions from the primary are considered.
What does "secondary" even mean, if packages from secondary sources are considered although a primary source has met a requirement already?
The implication of this MR would be that if they care about getting a particular version of a package, folk should say so in their pyproject.toml
. But if they've asked for a broad requirement that a primary source can satisfy: they should not expect poetry to go and look in secondary sources.
To expand on the trade that is being made here. This MR can introduce failures to find a solution in a case like this:
- the immediate requirement is for any version of
foo
- primary source provides only
foo 1.0
, so that's all that we consider - later in the search we encounter something in conflict with
foo 1.0
- even though a secondary source has
foo 2.0
which would have been fine: too bad, we now don't find that
The implicit claim of this MR is that this is an unusual case, which anyway can easily be solved eg by declaring that the requirement on foo
is >=2.0
, or by specifying the source for foo
.
Most of the time when people are using secondary sources, surely it's because they want to get most of their packages from one place (probably PyPI), but they have a small number of private or otherwise special packages in some secondary place. This MR prioritises that use case.
While I don't have data to back that, I would tend to agree with @dimbleby, as I believe that the main usage of secondary
is to declare private dependencies, or additional public ones not available on PyPI, for specific packages, for which users would generally declare the source in the dependency definitions.
If people still want the custom index to be searched for for all dependencies, as you report @radoering, wouldn't not setting secondary
achieve that? Although the slight difference is that the custom index would take precedence over PyPI, but it possibly may be fine for most people.
Personally, before needing to use a custom index as secondary
for private packages, I would have expected it to only be searched for for dependencies that explicitly declare it as their source, and was kind of surprised that it was not the case (and there's no way to work around that, AFAIK).
Even if there is a need to keep the existing behavior for some users, I still think that the changes in this PR are a more sensible default, since this is what, IMO, most people would expect.
I agree about the main use case. However, another common use case is to have patched/modified versions of public available packages in your private repository. Especially, if these patches/modifications are not required for the latest version but only for some older ones and these are transient dependencies, this change might make things more complicated. Probably, this use case can still be covered by being more explicit.
If people still want the custom index to be searched for for all dependencies, as you report @radoering, wouldn't not setting
secondary
achieve that? Although the slight difference is that the custom index would take precedence over PyPI, but it possibly may be fine for most people.
I don't think so. That's exactly what is changed here. Secondary sources are not searched anymore if a dependency is found in the primary source. With the change you can't achieve searching all sources anymore. You have to be more explicit (which might be good or not) and pin a depdendency to a specific source.
That said I'm not completely against this change. I just have concerns it's a breaking change. If we decide to do this, we should at least consider having a setting that restores the old behavior.
What about an isolated = true
option for sources that are secondary = true
? This would only use the source for the packages that explicitly use that source and never use it for any others. Adding such an option would provide a backward compatible way to enforce this behaviour.
There are some private packages that I only ever want installed from a private repository, never from PyPI - everything else can come from PyPI. After all, someone could register some package with the same name as one of my private packages in PyPI and a version of ^999.0.0
. It's not just about performance, there's security too.
Semi-related, I had an issue recently where I have three packages referencing a private repository via a secondary source, everything else doesn't specify the source and I'd expect them to use the default, implicit pypi
source. Weirdly, however, I had a problem when I added django-q[sentry]
. The sentry
extra adds django-q-sentry
, but for some reason when updating the lock file it specified the source for django-q-sentry
as the secondary source. This later caused installation to fail as the package doesn't exist under the secondary source. django-q
was still pointing to the default source even though its extra package wasn't. I worked around this by adding an explicit dependency for django-q-sentry
with source = "pypi"
. But it seems to point toward a bug in this behaviour of searching in all repositories. Alas, I haven't managed to work out a minimal reproducer yet.
I would like to add that such a feature is important, specifically as a PyTorch user.
For a quick demo, I have some packages that need to be retrieved from pypi and some others (torch
and torchvision
) need to be retrieved from another source.
The pyproject.toml
looks like this:
[tool.poetry]
name = "test-package"
version = "0.1.0"
description = ""
authors = ["[email protected]"]
readme = "README.md"
[tool.poetry.dependencies]
python = ">=3.8,<3.11"
numpy = { version = "~1.23", source = "pypi" }
rich = { version = "12.5.1", source = "pypi" }
torchvision = { version = "~0.13.0+cu116", source = "pytorch"}
[[tool.poetry.source]]
name = "pytorch"
url = "https://download.pytorch.org/whl/cu116/"
secondary = true
[build-system]
requires = ["poetry-core>=1.1.0.b3"]
build-backend = "poetry.core.masonry.api"
It results with the following output:
For torchvision dependencies, it will also try to use the secondary source. Moreover, using the secondary source is extremely slow...
Having the proposed isolated
keyword may be interested to only restrict specific packages to use the secondary source, instead of looking at all the sources and all the packages during the install.
Poetry version : 1.2.0.b3 Poetry-core version : 1.1.0.b3 Python version : 3.8.13
Indeed, this is very important to us. As mentioned, on our side we want to use this to use private package and as mentioned, perf is really bad with secondaries (because the way the resolution is done with different APIs than the one's of pypi.
As long as a secondary is present somewhere, the performance drops completely
We patched poetry to have this behavior in: https://github.com/python-poetry/poetry/pull/5472
Here are our results:
Poetry 1.1.13 patched with https://github.com/python-poetry/poetry/pull/5472
poetry update -> ~30s
Poetry 1.1.14 (not patched)
poetry update -> ~1 min 30s
Poetry 1.2.0b3 (not patched)
poetry update -> ~1 min
What about an
isolated = true
option for sources that aresecondary = true
? This would only use the source for the packages that explicitly use that source and never use it for any others. Adding such an option would provide a backward compatible way to enforce this behaviour.There are some private packages that I only ever want installed from a private repository, never from PyPI - everything else can come from PyPI. After all, someone could register some package with the same name as one of my private packages in PyPI and a version of
^999.0.0
. It's not just about performance, there's security too.
This sounds like a reasonable proposal.
Regardless of what we decide, it is important to keep backward compatibility, so we could either:
- provide
isolated = true
as suggested, or something similar, to specify that the repository should not be searched in - not search into secondary repositories by default, but provide an option to maintain the current behavior -> since this proposal would be a breaking change, I don't see it landing in a patch version (and I don't think we want to delay
1.2
even more for that specific change, but core team may have a more precise opinion on that)
Both proposals make sense, as long as in the end, users have an option to either opt in for this new feature, or can go back to the current behavior, especially since right now, if users have no other choice than to rely on a secondary source, and this source is slow network-wise, this could drastically slow down dependencies resolution.
Personally, letting breaking changes concerns aside, I still think that not searching in secondary sources is a more sensible default, because I think that most of the time, people use secondary sources for specific packages (whether they are private, or because some libraries are not published to PyPI), and may even assume that this is actually what Poetry does.
Just noting that I'm not personally motivated to drive this one further - I never use secondary repositories myself, so taking this from a quick-n-dirty MR to a redesigned set of configuration options doesn't much interest me.
I think this has been useful in kicking the discussion and establishing that there is interest: but it will likely now need someone who cares more about this to step up and do some work if it is to go further.
A related problem with this, if someone doesn't have access to the secondary source it doesn't work at all, because it always needs to try all the sources.
In this example I didn't turn on the VPN so I didn't have access to the secondary source, but I didn't needed it. And when I tried to force apache-airflow to pipy, with source='pypi'
, it also breaks due to the derived packages trying to download from the secondary source.
pyproject.toml:
[tool.poetry.dependencies]
python = "~3.7.13"
apache-airflow = { version = "2.2.5", platform = "linux" }
[[tool.poetry.source]]
name = "external_source"
url = "https://nexus.___.io/repository/___-pypi/simple"
secondary = true
poetry lock:
Updating dependencies
Resolving dependencies... (0.7s)
RepositoryError
403 Client Error: Forbidden for url: https://nexus.___.io/repository/___-pypi/simple/apache-airflow/
at ~/.local/share/pypoetry/venv/lib/python3.8/site-packages/poetry/repositories/legacy_repository.py:393 in _get
389│ if response.status_code == 404:
390│ return
391│ response.raise_for_status()
392│ except requests.HTTPError as e:
→ 393│ raise RepositoryError(e)
394│
395│ if response.status_code in (401, 403):
396│ self._log(
397│ "Authorization error accessing {url}".format(url=response.url),
I think, to preserve backwards compatibility, the long-term path forward is probably two different options (in my example, supplemental
and private
). I would propose something like the following, as an overview of behaviors:
- No options means that the repository overlays PyPI, but we will still fall back to PyPI if necessary (e.g. suitable for internal mirrors),
pip --extra-index-url
-style. This is known internally as primary, and is the current behavior. -
default = true
means that the repository replaces PyPI,pip --index-url
-style. This is the current behavior. -
supplemental = true
means that this source is only consulted after a lookup in thedefault
(implicitly PyPI unless configured otherwise) and primary sources fail. -
private = true
means that the repo will only be considered for packages that are explicitly configured withsource =
, and should be preferred by users for private packages to avoid dependency confusion attacks.
secondary = true
would be kept around as a deprecated option (likely with a warning), and would maintain the legacy behavior of being searched exhaustively for backwards compatibility.
secondary = true
would be kept around as a deprecated option (likely with a warning), and would maintain the legacy behavior of being searched exhaustively for backwards compatibility.
What is the difference between that new supplemental
and the old secondary
? Is it that it is only consulted if a lookup actually fails, so no request is made during the discovery phase of the version solving?
supplemental
would only be consulted after a failure to find packages in the primary and default sources. Currently, secondary
repositories are unconditionally searched, even if another secondary (or default/primary) has results. We cannot change this behavior without breaking existing projects, but we can deprecate and migrate away from it.
@neersighted great proposal, that would be a huge progress! Is there a PR to watch/contribute to for these changes?
@neersighted @graipher this is a very good proposal and would save me lots of headache. What is the progress on this?
@neersighted @graipher this is a very good proposal and would save me lots of headache. What is the progress on this?
It is a fairly hairy refactor, in order to introduce these new behaviors while permitting backwards compatibility. It likely wouldn't be looked at until 1.4, and no one has expressed interest in implementing it yet.
@neersighted I understand. Thanks for the update!
Closing in favor of #6713
I think, to preserve backwards compatibility, the long-term path forward is probably two different options (in my example,
supplemental
andprivate
). I would propose something like the following, as an overview of behaviors:
- No options means that the repository overlays PyPI, but we will still fall back to PyPI if necessary (e.g. suitable for internal mirrors),
pip --extra-index-url
-style. This is known internally as primary, and is the current behavior.default = true
means that the repository replaces PyPI,pip --index-url
-style. This is the current behavior.supplemental = true
means that this source is only consulted after a lookup in thedefault
(implicitly PyPI unless configured otherwise) and primary sources fail.private = true
means that the repo will only be considered for packages that are explicitly configured withsource =
, and should be preferred by users for private packages to avoid dependency confusion attacks.
secondary = true
would be kept around as a deprecated option (likely with a warning), and would maintain the legacy behavior of being searched exhaustively for backwards compatibility.
I've been doing some preparatory work to implement this. I'm now at the point where I write tests that express the desired behavior and found out that the private
case needs to be made more explicit. Specifically: if a package is configured with source
and targets a private
repository, what does that mean for finding that package's dependencies?
Currently, such dependencies are not bound to the source, meaning that they may be retrieved from any configured repo including the source repo. Perhaps the most natural option is to, when we introduce private
repos and a package's source is set to that private repo, its dependencies can be retrieved from all repositories including the private repo that is the package source (and no other private repos). However, there are alternative scenarios as well. For example, only might suggest that dependencies may not consider any private
repo.
The first suggestion seems most appropriate. My private packages have private dependencies, so I'd expect that to work just fine. Still, I'd like to hear your thoughts before I make assumptions.
@b-kamphorst would you mind discussing this on #6713 instead? I'd like to centralize the discussion there.
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.