Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

6713 Introduce supplemental package source #6879

Merged
merged 8 commits into from
May 12, 2023
Merged

6713 Introduce supplemental package source #6879

merged 8 commits into from
May 12, 2023

Conversation

b-kamphorst
Copy link
Contributor

@b-kamphorst b-kamphorst commented Oct 24, 2022

Pull Request Check List

Resolves: #6713
Relates to: #7658

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

@b-kamphorst 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
Copy link
Contributor Author

b-kamphorst commented Oct 24, 2022

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
Copy link
Member

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
Copy link
Contributor Author

b-kamphorst commented Oct 25, 2022

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
Copy link
Member

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
Copy link
Contributor

dimbleby commented Oct 25, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

b-kamphorst commented Nov 22, 2022

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.

@b-kamphorst b-kamphorst marked this pull request as ready for review December 14, 2022 19:24
@radoering
Copy link
Member

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.

@neersighted
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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.

Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 10 to 13
default: bool = False
secondary: bool = False
supplemental: bool = False
explicit: bool = False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

poetry.pool.add_repository(
PyPiRepository(disable_cache=disable_cache), default, not default
PyPiRepository(disable_cache=disable_cache), priority=Priority.DEFAULT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
src/poetry/console/commands/source/show.py Outdated Show resolved Hide resolved
src/poetry/factory.py Outdated Show resolved Hide resolved
@radoering
Copy link
Member

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.

@b-kamphorst b-kamphorst marked this pull request as draft March 17, 2023 07:46
@Secrus
Copy link
Member

Secrus commented Apr 17, 2023

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

@Secrus Secrus added the status/waiting-on-response Waiting on response from author label Apr 17, 2023
@radoering
Copy link
Member

radoering commented Apr 17, 2023

#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
Copy link
Member

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

@radoering radoering mentioned this pull request Apr 30, 2023
@b-kamphorst
Copy link
Contributor Author

@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.

@b-kamphorst b-kamphorst requested a review from radoering May 1, 2023 22:59
@b-kamphorst b-kamphorst marked this pull request as ready for review May 1, 2023 22:59
@b-kamphorst b-kamphorst changed the title 6713 Replace the secondary source type with more granular types 6713 Introduce supplemental package source May 2, 2023
@radoering 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
@github-actions
Copy link

github-actions bot commented May 6, 2023

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

Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just some minor nitpicks.

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
@radoering radoering merged commit 372a181 into python-poetry:master May 12, 2023
36 checks passed
mwalbeck pushed a commit to mwalbeck/docker-python-poetry that referenced this pull request May 23, 2023
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#&#8203;150---2023-05-19)

[Compare Source](python-poetry/poetry@1.4.2...1.5.0)

##### Added

-   **Introduce the new source priorities `explicit` and `supplemental`** ([#&#8203;7658](python-poetry/poetry#7658),
    [#&#8203;6879](python-poetry/poetry#6879)).
-   **Introduce the option to configure the priority of the implicit PyPI source** ([#&#8203;7801](python-poetry/poetry#7801)).
-   Add handling for corrupt cache files ([#&#8203;7453](python-poetry/poetry#7453)).
-   Improve caching of URL and git dependencies ([#&#8203;7693](python-poetry/poetry#7693),
    [#&#8203;7473](python-poetry/poetry#7473)).
-   Add option to skip installing directory dependencies ([#&#8203;6845](python-poetry/poetry#6845),
    [#&#8203;7923](python-poetry/poetry#7923)).
-   Add `--executable` option to `poetry env info` ([#&#8203;7547](python-poetry/poetry#7547)).
-   Add `--top-level` option to `poetry show` ([#&#8203;7415](python-poetry/poetry#7415)).
-   Add `--lock` option to `poetry remove` ([#&#8203;7917](python-poetry/poetry#7917)).
-   Add experimental `POETRY_REQUESTS_TIMEOUT` option ([#&#8203;7081](python-poetry/poetry#7081)).
-   Improve performance of wheel inspection by avoiding unnecessary file copy operations ([#&#8203;7916](python-poetry/poetry#7916)).

##### Changed

-   **Remove the old deprecated installer and the corresponding setting `experimental.new-installer`** ([#&#8203;7356](python-poetry/poetry#7356)).
-   **Introduce `priority` key for sources and deprecate flags `default` and `secondary`** ([#&#8203;7658](python-poetry/poetry#7658)).
-   Deprecate `poetry run <entry point>` if the entry point was not previously installed via `poetry install` ([#&#8203;7606](python-poetry/poetry#7606)).
-   Only write the lock file if the installation succeeds ([#&#8203;7498](python-poetry/poetry#7498)).
-   Do not write the unused package category into the lock file ([#&#8203;7637](python-poetry/poetry#7637)).

##### Fixed

-   Fix an issue where Poetry's internal pyproject.toml continually grows larger with empty lines ([#&#8203;7705](python-poetry/poetry#7705)).
-   Fix an issue where Poetry crashes due to corrupt cache files ([#&#8203;7453](python-poetry/poetry#7453)).
-   Fix an issue where the `Retry-After` in HTTP responses was not respected and retries were handled inconsistently ([#&#8203;7072](python-poetry/poetry#7072)).
-   Fix an issue where Poetry silently ignored invalid groups ([#&#8203;7529](python-poetry/poetry#7529)).
-   Fix an issue where Poetry does not find a compatible Python version if not given explicitly ([#&#8203;7771](python-poetry/poetry#7771)).
-   Fix an issue where the `direct_url.json` of an editable install from a git dependency was invalid ([#&#8203;7473](python-poetry/poetry#7473)).
-   Fix an issue where error messages from build backends were not decoded correctly ([#&#8203;7781](python-poetry/poetry#7781)).
-   Fix an infinite loop when adding certain dependencies ([#&#8203;7405](python-poetry/poetry#7405)).
-   Fix an issue where pre-commit hooks skip pyproject.toml files in subdirectories ([#&#8203;7239](python-poetry/poetry#7239)).
-   Fix an issue where pre-commit hooks do not use the expected Python version ([#&#8203;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 ([#&#8203;7757](python-poetry/poetry#7757)).
-   Fix an issue where `poetry install` returns a zero exit status even though the build script failed ([#&#8203;7812](python-poetry/poetry#7812)).
-   Fix an issue where an existing `.venv` was not used if `in-project` was not set ([#&#8203;7792](python-poetry/poetry#7792)).
-   Fix an issue where multiple extras passed to `poetry add` were not parsed correctly ([#&#8203;7836](python-poetry/poetry#7836)).
-   Fix an issue where `poetry shell` did not send a newline to `fish` ([#&#8203;7884](python-poetry/poetry#7884)).
-   Fix an issue where `poetry update --lock` printed operations that were not executed ([#&#8203;7915](python-poetry/poetry#7915)).
-   Fix an issue where `poetry add --lock` did perform a full update of all dependencies ([#&#8203;7920](python-poetry/poetry#7920)).
-   Fix an issue where `poetry shell` did not work with `nushell` ([#&#8203;7919](python-poetry/poetry#7919)).
-   Fix an issue where subprocess calls failed on Python 3.7 ([#&#8203;7932](python-poetry/poetry#7932)).
-   Fix an issue where keyring was called even though the password was stored in an environment variable ([#&#8203;7928](python-poetry/poetry#7928)).

##### Docs

-   Add information about what to use instead of `--dev` ([#&#8203;7647](python-poetry/poetry#7647)).
-   Promote semantic versioning less aggressively ([#&#8203;7517](python-poetry/poetry#7517)).
-   Explain Poetry's own versioning scheme in the FAQ ([#&#8203;7517](python-poetry/poetry#7517)).
-   Update documentation for configuration with environment variables ([#&#8203;6711](python-poetry/poetry#6711)).
-   Add details how to disable the virtualenv prompt ([#&#8203;7874](python-poetry/poetry#7874)).
-   Improve documentation on whether to commit `poetry.lock` ([#&#8203;7506](python-poetry/poetry#7506)).
-   Improve documentation of `virtualenv.create` ([#&#8203;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 ([#&#8203;569](python-poetry/poetry-core#569)).
-   Increase robustness when deleting temporary directories on Windows ([#&#8203;460](python-poetry/poetry-core#460)).
-   Replace `tomlkit` with `tomli`, which changes the interface of some *internal* classes ([#&#8203;483](python-poetry/poetry-core#483)).
-   Deprecate `Package.category` ([#&#8203;561](python-poetry/poetry-core#561)).
-   Fix a performance regression in marker handling ([#&#8203;568](python-poetry/poetry-core#568)).
-   Fix an issue where wildcard version constraints were not handled correctly ([#&#8203;402](python-poetry/poetry-core#402)).
-   Fix an issue where `poetry build` created duplicate Python classifiers if they were specified manually ([#&#8203;578](python-poetry/poetry-core#578)).
-   Fix an issue where local versions where not handled correctly ([#&#8203;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.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS44Mi4wIiwidXBkYXRlZEluVmVyIjoiMzUuODIuMCIsInRhcmdldEJyYW5jaCI6Im1hc3RlciJ9-->

Reviewed-on: https://git.walbeck.it/walbeck-it/docker-python-poetry/pulls/717
Co-authored-by: renovate-bot <[email protected]>
Co-committed-by: renovate-bot <[email protected]>
Copy link

github-actions bot commented Mar 3, 2024

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/sources Releated to package sources/indexes/repositories impact/changelog Requires a changelog entry impact/docs Contains or requires documentation changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace the secondary source type with more granular types
5 participants