6713 Introduce supplemental package source #6879

b-kamphorst · 2022-10-24T21:58:34Z

Pull Request Check List

Resolves: #6713
Relates to: #7658

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

b-kamphorst · 2022-10-24T22:00:48Z

This is still work in progress. In particular, the following items are on my TODO list:

add tests to tests/test_factory (incl. adding fixtures)
add tests to tests/commands/source/test_add.py
add behaviour to source add
update documentation poetry source add

For backwards compatibility I complemented the default and secondary keys for sources in pyproject.toml with supplemental and private. However it seems more maintainable to introduce a non-boolean key priority. How do you feel about the suggestion (and its relation to this PR)?

neersighted · 2022-10-24T22:08:59Z

I don't think integer priorities make sense as there are specific semantics beyond order that some of the flags indicate (e.g. default and private have meanings beyond the lookup order).

b-kamphorst · 2022-10-25T07:10:53Z

I don't think integer priorities make sense as there are specific semantics beyond order that some of the flags indicate (e.g. default and private have meanings beyond the lookup order).

I agree. I was thinking of literals; e.g.

[[tool.poetry.source]]
name = "foo"
url = "https://foo.bar/simple/"
type = "supplemental " # type = "primary" if unspecified

instead of

[[tool.poetry.source]]
name = "foo"
url = "https://foo.bar/simple/"
default = false
secondary = false
supplemental = true
private = false

Would that work?

neersighted · 2022-10-25T14:25:51Z

Ah, that makes more sense -- I read that as integer, but I see that you didn't explicitly state that.

I'm not necessarily opposed, but I think we would need to think about the design and how it would interact with older Poetry versions (maybe it's time to add a minimum-poetry-version gate?), as well as the potential indexed = true from #5442 and if we can easily validate the string-constants and provide helpful errors (I think our JSON schema lets us do this trivially).

dimbleby · 2022-10-25T18:29:44Z

Haven't got any further than starting to read the docs yet: but it looks as though private is not a good name here. Other types of source might be private, and private sources might not be private. The name is completely disconnected from the meaning of the word!

explicit maybe?

dimbleby · 2022-10-25T18:38:45Z

this reminds me that I still think it's a good idea to spell out what key use cases you're trying to cover here, and how this approach would meet them - #6713 (comment)

in particular two of the cases mentioned there exactly show what I mean about private vs "private"

a private mirror from which I want to get everything is private but not private
the pytorch repository from which folk want to get pytorch only is private but not private

b-kamphorst · 2022-10-30T20:51:58Z

Haven't got any further than starting to read the docs yet: but it looks as though private is not a good name here. Other types of source might be private, and private sources might not be private. The name is completely disconnected from the meaning of the word!

explicit maybe?

I quite like explicit, I'll rename if @neersighted agrees.

b-kamphorst · 2022-11-15T23:54:01Z

I've continued the effort through the cli and factory. I expect that there is some test to actually verify that the correct information ends up in pyproject.toml, but I have not found it yet. Feel free to point me in the right direction!

Feedback is welcome. Note that there are some open discussion that need to be concluded, e.g. #6879 (comment) and #6713 (comment).

b-kamphorst · 2022-11-22T23:02:49Z

Tests for poetry-plugin-export are failing (as in my previous MR). The obvious conflicts are in the code (here) and tests (here and here). These can easily be fixed, but we end up with a circular dependency (plugin will fail for current version of poetry, new poetry breaks plugin). Thoughts?

NTS: also add tests for Priority.EXPLICIT in poetry-plugin-export.

radoering · 2023-01-11T17:01:23Z

Is there a discussion somewhere why we should change the priority of default and primary?

I expect that there is some test to actually verify that the correct information ends up in pyproject.toml, but I have not found it yet.

I'd just deliberately introduce a bug, run the tests locally and see if some tests fail. If all tests pass, then there is no (sufficient) test. 😉

Note that there are some open discussion that need to be concluded, e.g. #6879 (comment) and #6713 (comment).

explicit maybe?

IMO, explicit is fine.

is supplemental behaviour actually breaking for secondary? If secondary sources are considered after all other (primary, default) sources, then the resolution behaviour should be identical (apart from making / timing of queries). If not, what am I missing?

With secondary, you can get an additional version of a package, which is not available in primary sources and resolution might choose exactly that version.

src/poetry/repositories/repository_pool.py

neersighted · 2023-01-23T22:20:29Z

Just checking in to say sorry for going AWOL; this takes more energy than I have the ability to contribute now due to other commitments. You're in good hands with @radoering and I'll try to provide input again when I am able.

b-kamphorst · 2023-01-23T22:32:33Z

Is there a discussion somewhere why we should change the priority of default and primary?

This is actually a bug. primary sources are supposed to behave as the pip --extra-index-url, which have priority over --index-url -- our default. However, it turns out that the bug fix is incompatible with the poetry-plugin-export (this line). The method RepositoryPool.get_priority will make it easy to change that line in a separate PR.

I propose to merge this PR in poetry without fixing the priority, then updating poetry-plugin-export to not depend on the order of repositories in pool.repositories but rather just verify the priority of a given repo, and then fix the bug in poetry.

I'd just deliberately introduce a bug, run the tests locally and see if some tests fail. If all tests pass, then there is no (sufficient) test. 😉

TODO on my side.

IMO, explicit is fine.

OK!

With secondary, you can get an additional version of a package, which is not available in primary sources and resolution might choose exactly that version.

Ah you are right. I got confused between the version pickers (there is one here and another here) and I mistakenly thought that one of them did not sort and as such would just pick the first returned version (which should be a non-secondary source when available).

b-kamphorst · 2023-01-23T22:34:17Z

Just checking in to say sorry for going AWOL; this takes more energy than I have the ability to contribute now due to other commitments. You're in good hands with @radoering and I'll try to provide input again when I am able.

No worries, I appreciate both of your feedback and hope that I can help poetry with this PR :)

b-kamphorst · 2023-01-23T22:46:26Z

I'd just deliberately introduce a bug, run the tests locally and see if some tests fail. If all tests pass, then there is no (sufficient) test. 😉

Done -- all works!

@radoering I think this PR is ready for another round of review. I'll probably have to remove the DEFAULT-PRIMARY-changing commits and you may prefer some other tidying up as well. Just let me know what you need for approval.

radoering · 2023-01-25T17:35:37Z

This is actually a bug. primary sources are supposed to behave as the pip --extra-index-url, which have priority over --index-url -- our default.

IMO that's not that clear. Actually, there is no warranted order of --extra-index-url and --index-url. IIUC, pip considers all sources equal and it's an implementation detail which source is chosen first. See pypa/pip#9610 (comment) for example. That's why I doubt whether we should change order at all.

I propose to merge this PR in poetry without fixing the priority

I absolutely agree on this point.

I'll try to do a review this week or next.

radoering

I haven't looked at the tests yet, but I think there are some implementation details that need to be worked out first.

Coming back to the default priority: I think the issue is that at the moment (state of current master) we are inconsistent between implementation and documentation:

implementation	documentation
If there is no explicit `default`, PyPI is only the `default` if there is no `primary`, otherwise it's `secondary`. `default` is the highest priority. In other words, implicit PyPI always comes after `primary` but is not always the `default`.	It's a bit difficult to find but it says that primary sources take precedence over PyPI and that the default source takes precedence over `secondary` sources. It does not clearly say but implies that if there is no explicit `default`, PyPI always is the `default`. If you combine everything, it seems `primary` sources should take precedence over the `default` source (even if it's not PyPI).

This inconsistence makes it difficult to decide if changes made in this PR are correct or not. I'm still not sure what to consider the source of truth. I think it's difficult to teach the current implemented behavior. Thus, it might make sense to consider the documentation as correct and change the implementation so that default is in between primary and secondary. If we consider changing the order a bugfix, we should do this in a separate PR (maybe even before this PR). I'll try to discuss this in the next maintainer meeting and give you some feedback.

radoering · 2023-01-28T09:50:28Z

src/poetry/config/source.py

+    default: bool = False
+    secondary: bool = False
+    supplemental: bool = False
+    explicit: bool = False


Since priorities exclude each other, I think we shouldn't have five different settings and users wondering what happens if they combine several of them. Thus, I'd propose to only keep the existing settings and print a deprecation warning if they are set. Further, introduce a new enum type "priority" in the schema. The deprecated settings should only have an effect if priority is not set.

Well that was quite a headache, but educative as well. I look forward to your thoughts on the last two commits, which introduce the enum.

radoering · 2023-01-28T09:59:59Z

src/poetry/factory.py

            poetry.pool.add_repository(
-                PyPiRepository(disable_cache=disable_cache), default, not default
+                PyPiRepository(disable_cache=disable_cache), priority=Priority.DEFAULT


To keep existing behavior, the priority has to be SECONDARY if there are primary repositories. I think it makes sense to keep this behavior because otherwise you can't add sources with higher priority than PyPI without deactivating it. However, I'm not that sure anymore whether we should keep default before primary or change the order (in another PR). It probably makes most sense to wait for a decision before changing anything here.

Hi @radoering, do you have any updates on the requested behaviour of the PyPI repository?

Unfortunately, not yet. I wanted to discuss this topic in the maintainer meeting, which is scheduled every two weeks, but the last meeting was cancelled. Thus, I'll try in the next meeting.

See #6713 (comment) for the results of the discussion in our meeting. I'd like to hear your opinion on the two variants.

src/poetry/console/commands/source/add.py

src/poetry/console/commands/source/show.py

src/poetry/factory.py

src/poetry/json/schemas/poetry.json

radoering · 2023-03-08T05:51:37Z

May you split the introduction of explicit into a separate PR as first step? I think that's the most uncontroversial part since it's not influenced by order or priority of any other repository. (Maybe, we can switch from flags to one setting as proposed in #6879 (comment) in this PR, too.)

Another reason I like to see explicit first is that I'm not quite convinced by #7430 and want to try an alternative approach that requires explicit.

Secrus · 2023-04-17T10:33:51Z

Is this still a valid PR since #7658 was merged? If not, please close.

radoering · 2023-04-17T14:29:55Z

#7658 was only part of this one. Introducing supplemental is still open. However, it might be easier to close this one and open a new PR? Further, it might make sense to wait for #7801 to avoid conflicts.

radoering · 2023-04-27T03:58:36Z

@b-kamphorst Since #7801 has been merged, we can go for supplemental now.

b-kamphorst · 2023-05-01T22:57:32Z

@b-kamphorst Since #7801 has been merged, we can go for supplemental now.

I've rebased the MR on master. Most looks fine to me, but surely a fresh review is welcome.

github-actions · 2023-05-06T11:46:40Z

Deploy preview for website ready!

✅ Preview
https://website-njpk7uaj6-python-poetry.vercel.app

Built with commit 4e99bff.
This pull request is being automatically deployed with vercel-action

radoering

Looks good. Just some minor nitpicks.

tests/test_factory.py

src/poetry/console/commands/source/add.py

src/poetry/factory.py

This PR contains the following updates: | Package | Update | Change | |---|---|---| | [poetry](https://python-poetry.org/) ([source](https://github.com/python-poetry/poetry), [changelog](https://python-poetry.org/history/)) | minor | `1.4.2` -> `1.5.0` | --- ### Release Notes <details> <summary>python-poetry/poetry</summary> ### [`v1.5.0`](https://github.com/python-poetry/poetry/blob/HEAD/CHANGELOG.md#150---2023-05-19) [Compare Source](python-poetry/poetry@1.4.2...1.5.0) ##### Added - **Introduce the new source priorities `explicit` and `supplemental`** ([#7658](python-poetry/poetry#7658), [#6879](python-poetry/poetry#6879)). - **Introduce the option to configure the priority of the implicit PyPI source** ([#7801](python-poetry/poetry#7801)). - Add handling for corrupt cache files ([#7453](python-poetry/poetry#7453)). - Improve caching of URL and git dependencies ([#7693](python-poetry/poetry#7693), [#7473](python-poetry/poetry#7473)). - Add option to skip installing directory dependencies ([#6845](python-poetry/poetry#6845), [#7923](python-poetry/poetry#7923)). - Add `--executable` option to `poetry env info` ([#7547](python-poetry/poetry#7547)). - Add `--top-level` option to `poetry show` ([#7415](python-poetry/poetry#7415)). - Add `--lock` option to `poetry remove` ([#7917](python-poetry/poetry#7917)). - Add experimental `POETRY_REQUESTS_TIMEOUT` option ([#7081](python-poetry/poetry#7081)). - Improve performance of wheel inspection by avoiding unnecessary file copy operations ([#7916](python-poetry/poetry#7916)). ##### Changed - **Remove the old deprecated installer and the corresponding setting `experimental.new-installer`** ([#7356](python-poetry/poetry#7356)). - **Introduce `priority` key for sources and deprecate flags `default` and `secondary`** ([#7658](python-poetry/poetry#7658)). - Deprecate `poetry run <entry point>` if the entry point was not previously installed via `poetry install` ([#7606](python-poetry/poetry#7606)). - Only write the lock file if the installation succeeds ([#7498](python-poetry/poetry#7498)). - Do not write the unused package category into the lock file ([#7637](python-poetry/poetry#7637)). ##### Fixed - Fix an issue where Poetry's internal pyproject.toml continually grows larger with empty lines ([#7705](python-poetry/poetry#7705)). - Fix an issue where Poetry crashes due to corrupt cache files ([#7453](python-poetry/poetry#7453)). - Fix an issue where the `Retry-After` in HTTP responses was not respected and retries were handled inconsistently ([#7072](python-poetry/poetry#7072)). - Fix an issue where Poetry silently ignored invalid groups ([#7529](python-poetry/poetry#7529)). - Fix an issue where Poetry does not find a compatible Python version if not given explicitly ([#7771](python-poetry/poetry#7771)). - Fix an issue where the `direct_url.json` of an editable install from a git dependency was invalid ([#7473](python-poetry/poetry#7473)). - Fix an issue where error messages from build backends were not decoded correctly ([#7781](python-poetry/poetry#7781)). - Fix an infinite loop when adding certain dependencies ([#7405](python-poetry/poetry#7405)). - Fix an issue where pre-commit hooks skip pyproject.toml files in subdirectories ([#7239](python-poetry/poetry#7239)). - Fix an issue where pre-commit hooks do not use the expected Python version ([#6989](python-poetry/poetry#6989)). - Fix an issue where an unclear error message is printed if the project name is the same as one of its dependencies ([#7757](python-poetry/poetry#7757)). - Fix an issue where `poetry install` returns a zero exit status even though the build script failed ([#7812](python-poetry/poetry#7812)). - Fix an issue where an existing `.venv` was not used if `in-project` was not set ([#7792](python-poetry/poetry#7792)). - Fix an issue where multiple extras passed to `poetry add` were not parsed correctly ([#7836](python-poetry/poetry#7836)). - Fix an issue where `poetry shell` did not send a newline to `fish` ([#7884](python-poetry/poetry#7884)). - Fix an issue where `poetry update --lock` printed operations that were not executed ([#7915](python-poetry/poetry#7915)). - Fix an issue where `poetry add --lock` did perform a full update of all dependencies ([#7920](python-poetry/poetry#7920)). - Fix an issue where `poetry shell` did not work with `nushell` ([#7919](python-poetry/poetry#7919)). - Fix an issue where subprocess calls failed on Python 3.7 ([#7932](python-poetry/poetry#7932)). - Fix an issue where keyring was called even though the password was stored in an environment variable ([#7928](python-poetry/poetry#7928)). ##### Docs - Add information about what to use instead of `--dev` ([#7647](python-poetry/poetry#7647)). - Promote semantic versioning less aggressively ([#7517](python-poetry/poetry#7517)). - Explain Poetry's own versioning scheme in the FAQ ([#7517](python-poetry/poetry#7517)). - Update documentation for configuration with environment variables ([#6711](python-poetry/poetry#6711)). - Add details how to disable the virtualenv prompt ([#7874](python-poetry/poetry#7874)). - Improve documentation on whether to commit `poetry.lock` ([#7506](python-poetry/poetry#7506)). - Improve documentation of `virtualenv.create` ([#7608](python-poetry/poetry#7608)). ##### poetry-core ([`1.6.0`](https://github.com/python-poetry/poetry-core/releases/tag/1.6.0)) - Improve error message for invalid markers ([#569](python-poetry/poetry-core#569)). - Increase robustness when deleting temporary directories on Windows ([#460](python-poetry/poetry-core#460)). - Replace `tomlkit` with `tomli`, which changes the interface of some *internal* classes ([#483](python-poetry/poetry-core#483)). - Deprecate `Package.category` ([#561](python-poetry/poetry-core#561)). - Fix a performance regression in marker handling ([#568](python-poetry/poetry-core#568)). - Fix an issue where wildcard version constraints were not handled correctly ([#402](python-poetry/poetry-core#402)). - Fix an issue where `poetry build` created duplicate Python classifiers if they were specified manually ([#578](python-poetry/poetry-core#578)). - Fix an issue where local versions where not handled correctly ([#579](python-poetry/poetry-core#579)). </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://git.walbeck.it/walbeck-it/docker-python-poetry/pulls/717 Co-authored-by: renovate-bot <bot@walbeck.it> Co-committed-by: renovate-bot <bot@walbeck.it>

github-actions · 2024-03-03T22:04:53Z

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.

b-kamphorst changed the title ~~5959 introduce new modes for package source resolution~~ 6713 Replace the secondary source type with more granular types Oct 24, 2022

b-kamphorst marked this pull request as draft October 24, 2022 22:02

b-kamphorst mentioned this pull request Oct 24, 2022

Replace the secondary source type with more granular types #6713

Closed

b-kamphorst marked this pull request as ready for review December 14, 2022 19:24

radoering reviewed Jan 11, 2023

View reviewed changes

src/poetry/repositories/repository_pool.py Outdated Show resolved Hide resolved

radoering reviewed Jan 28, 2023

View reviewed changes

radoering mentioned this pull request Jan 29, 2023

repositories: deprecate default option and introduce possibility to disable PyPI explicitly #7430

Closed

2 tasks

b-kamphorst commented Feb 1, 2023

View reviewed changes

src/poetry/factory.py Outdated Show resolved Hide resolved

b-kamphorst commented Feb 1, 2023

View reviewed changes

src/poetry/json/schemas/poetry.json Outdated Show resolved Hide resolved

b-kamphorst mentioned this pull request Mar 17, 2023

6713 Introduce explicit package source #7658

Merged

2 tasks

b-kamphorst marked this pull request as draft March 17, 2023 07:46

Secrus added the status/waiting-on-response Waiting on response from author label Apr 17, 2023

radoering mentioned this pull request Apr 30, 2023

1.5.0 Release #7853

Closed

b-kamphorst requested a review from radoering May 1, 2023 22:59

b-kamphorst marked this pull request as ready for review May 1, 2023 22:59

b-kamphorst changed the title ~~6713 Replace the secondary source type with more granular types~~ 6713 Introduce supplemental package source May 2, 2023

radoering added impact/changelog Requires a changelog entry impact/docs Contains or requires documentation changes area/sources Releated to package sources/indexes/repositories and removed status/waiting-on-response Waiting on response from author labels May 6, 2023

radoering requested changes May 6, 2023

View reviewed changes

tests/test_factory.py Show resolved Hide resolved

src/poetry/console/commands/source/add.py Outdated Show resolved Hide resolved

src/poetry/factory.py Outdated Show resolved Hide resolved

src/poetry/factory.py Outdated Show resolved Hide resolved

b-kamphorst added 8 commits May 12, 2023 16:44

test: add tests Priority.SUPPLEMENTARY repositories

fc652b5

feat: Priority.SUPPLEMENTAL passes all tests

6bca72d

docs: introduce supplemental sources

7f62280

feat: incorporate Priority.SUPPLEMENTAL in source add and source show

3ce2812

test: ensure factory understands Priority.SUPPLEMENTAL

59e1437

feat: add Priority.SUPPLEMENTAL to poetry schema

bc4f04d

deprecate Priority.SECONDARY

8ac3b33

style: incorporate review suggestions

4e99bff

radoering approved these changes May 12, 2023

View reviewed changes

radoering merged commit 372a181 into python-poetry:master May 12, 2023

radoering mentioned this pull request May 18, 2023

release: bump version to 1.5.0 #7947

Merged

b-kamphorst mentioned this pull request May 23, 2023

7988 fix resolving subdependencies from explicit source #7992

Closed

2 tasks

github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

6713 Introduce supplemental package source #6879

6713 Introduce supplemental package source #6879

b-kamphorst commented Oct 24, 2022 •

edited

Loading

b-kamphorst commented Oct 24, 2022 •

edited

Loading

neersighted commented Oct 24, 2022

b-kamphorst commented Oct 25, 2022 •

edited

Loading

neersighted commented Oct 25, 2022

dimbleby commented Oct 25, 2022 •

edited

Loading

dimbleby commented Oct 25, 2022

b-kamphorst commented Oct 30, 2022

b-kamphorst commented Nov 15, 2022

b-kamphorst commented Nov 22, 2022 •

edited

Loading

radoering commented Jan 11, 2023

neersighted commented Jan 23, 2023

b-kamphorst commented Jan 23, 2023

b-kamphorst commented Jan 23, 2023

b-kamphorst commented Jan 23, 2023

radoering commented Jan 25, 2023

radoering left a comment •

edited

Loading

radoering Jan 28, 2023

b-kamphorst Feb 1, 2023

radoering Jan 28, 2023

b-kamphorst Feb 13, 2023

radoering Feb 14, 2023

radoering Feb 18, 2023

radoering commented Mar 8, 2023

Secrus commented Apr 17, 2023

radoering commented Apr 17, 2023 •

edited

Loading

radoering commented Apr 27, 2023

b-kamphorst commented May 1, 2023

github-actions bot commented May 6, 2023 •

edited

Loading

radoering left a comment

github-actions bot commented Mar 3, 2024

6713 Introduce supplemental package source #6879

6713 Introduce supplemental package source #6879

Conversation

b-kamphorst commented Oct 24, 2022 • edited Loading

Pull Request Check List

b-kamphorst commented Oct 24, 2022 • edited Loading

neersighted commented Oct 24, 2022

b-kamphorst commented Oct 25, 2022 • edited Loading

neersighted commented Oct 25, 2022

dimbleby commented Oct 25, 2022 • edited Loading

dimbleby commented Oct 25, 2022

b-kamphorst commented Oct 30, 2022

b-kamphorst commented Nov 15, 2022

b-kamphorst commented Nov 22, 2022 • edited Loading

radoering commented Jan 11, 2023

neersighted commented Jan 23, 2023

b-kamphorst commented Jan 23, 2023

b-kamphorst commented Jan 23, 2023

b-kamphorst commented Jan 23, 2023

radoering commented Jan 25, 2023

radoering left a comment • edited Loading

Choose a reason for hiding this comment

radoering Jan 28, 2023

Choose a reason for hiding this comment

b-kamphorst Feb 1, 2023

Choose a reason for hiding this comment

radoering Jan 28, 2023

Choose a reason for hiding this comment

b-kamphorst Feb 13, 2023

Choose a reason for hiding this comment

radoering Feb 14, 2023

Choose a reason for hiding this comment

radoering Feb 18, 2023

Choose a reason for hiding this comment

radoering commented Mar 8, 2023

Secrus commented Apr 17, 2023

radoering commented Apr 17, 2023 • edited Loading

radoering commented Apr 27, 2023

b-kamphorst commented May 1, 2023

github-actions bot commented May 6, 2023 • edited Loading

radoering left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 3, 2024

b-kamphorst commented Oct 24, 2022 •

edited

Loading

b-kamphorst commented Oct 24, 2022 •

edited

Loading

b-kamphorst commented Oct 25, 2022 •

edited

Loading

dimbleby commented Oct 25, 2022 •

edited

Loading

b-kamphorst commented Nov 22, 2022 •

edited

Loading

radoering left a comment •

edited

Loading

radoering commented Apr 17, 2023 •

edited

Loading

github-actions bot commented May 6, 2023 •

edited

Loading