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

Add option to skip installing directory dependencies #6845

Merged
merged 23 commits into from
Apr 10, 2023

Conversation

adriangb
Copy link
Contributor

@adriangb adriangb commented Oct 20, 2022

This serves the same purpose as --only-root but for path deps (which just like the "root" will bust caches if any source file changes).

If/once python-poetry/poetry-core#520 gets merged this will allow writing a Dockerfile along the lines of:

FROM python

COPY pyproject.toml poetry.lock .
RUN pip install poetry && poetry install --no-root --no-path
COPY src/ ./src
COPY folder-of-path-deps/ folder-of-path-deps/
RUN poetry install

I chose --no-path because of it's parallels to --no-root, but I'm open to other options.

Closes #6845
Closes #6850
Closes #3671

docs/faq.md Outdated Show resolved Hide resolved
@adriangb
Copy link
Contributor Author

adriangb commented Nov 5, 2022

I think this fixes #668

@adriangb
Copy link
Contributor Author

@radoering while we wait for review on python-poetry/poetry-core#520, would you mind taking a look at this and giving some thoughts? Thanks!

@radoering
Copy link
Member

If I understand correctly, the main motivation for this feature is the Docker cache. Before taking a look into the implementation details, I'd like to here the opinions of other maintainers if this feature makes sense for them. Especially @neersighted because he has much more experience with Docker than I do.

Just one question: Does it make sense to lump directory and file dependencies together (since both are path dependencies) or should it only consider directory dependencies?

@adriangb
Copy link
Contributor Author

Just one question: Does it make sense to lump directory and file dependencies together (since both are path dependencies) or should it only consider directory dependencies?

Great question. I’m not super familiar with workflows around file dependencies, I’ve never used them. The fundamental question as far as caches are concerned is “does this likely change every build (source code) or only rarely (dependencies)?”.
The main use case I associate file dependencies with is vendoring, in which case they’re not changing any more often than other leaf dependencies. What makes directory dependencies special is that they’re often actually your own source code.
We could always add another flag allowing users to choose depending on their use case. It’s a bit more complexity but avoids painting ourselves into a corner if we miss some use cases.

@neersighted
Copy link
Member

I'm not convinced this is useful, as in the use-case shown here (not building a package, just using Poetry to set up an environment), groups could easily be used. I don't think path dependencies should be special at all; encouraging users to think of them as 'special' takes us down a path we have strictly wanted to avoid (e.g. leaking metadata beyond the Core Metadata spec between layers of the dependency tree because Poetry is used all the way down), and reduces the interoperability of Poetry with the rest of the ecosystem.

I think that documenting the use of this pattern, but with groups, can help the people who are trying to do monorepos today, and that a workspace-like approach like suggested in #2270 is what we should explore, instead of trying to tear down the Core Metadata/PEP 517 wall between Poetry projects.

@adriangb
Copy link
Contributor Author

adriangb commented Jan 22, 2023

in the use-case shown here (not building a package, just using Poetry to set up an environment), groups could easily be used

could you explain how? I use dependency groups in conjunction with path dependencies (each group is just a path dependency thus allowing selecting/deselecting subprojects; see https://github.com/adriangb/python-monorepo/tree/main/poetry) in a real world monorepo and it works great aside from docker caching. I don’t see how “using dependency groups” solves the same problem this PR does?

@neersighted
Copy link
Member

neersighted commented Jan 22, 2023

Hmm, I did not realize that this PR was targeted at the pattern in your example repo there. While I brought up the overall direction for better supporting monorepos as kind of a extension of my thinking re: this PR, I did not realize that is at the core of what you want to do.

I think that makes my objection firmer -- special support for using path dependencies in this way (with a superproject that delegates to subprojects) is not desirable because it is ultimately incomplete unless we tear down the metadata barrier, and all the designs along that line have been rejected to date. It's possible we decide that the subproject approach is not tenable and go that way after all, but I think it would have to be comprehensive and not piecemeal. It would probably be better if it was a new dependency type if that were the case, instead of overloading path =, but I digress.

Regarding the use case you have, I'm afraid I still don't see how what you want to do is not possible with dependency groups. Could one not do the following (once python-poetry/poetry-core#520 is in, with all your groups set to optional)?

FROM python:3.11

COPY pyproject.toml poetry.lock .
RUN pip install poetry && poetry install --no-root
COPY src/ ./src
RUN poetry install --only-root
COPY folder-of-path-deps/ folder-of-path-deps/
RUN poetry install --no-root --with <path-dep-groups>

@adriangb
Copy link
Contributor Author

special support for using path dependencies in this way (with a superproject that delegates to subprojects) is not desirable because it is ultimately incomplete unless we tear down the metadata barrier, and all the designs along that line have been rejected to date

I haven’t heard any objection to my design. If you can think of any issues let's discuss in #6850.

I think it would have to be comprehensive and not piecemeal

Yes this is a small isolated piece, but that is by design. Part of the attractive of this design is that it can be implemented in small incremental steps, most of which except maybe this one are already completed and we’re beneficial to Poetry regardless of the acceptance of the proposed monorepo design. So I wouldn’t really call it “piecemeal”.

I'm afraid I still don't see how what you want to do is not possible with dependency groups. Could one not do the following (once python-poetry/poetry-core#520 is in, with all your groups set to optional)?

I don’t think things will work properly without this PR but I’m away from a computer to confirm, I can test later or feel free to give it a crack with my example repo.

@adriangb
Copy link
Contributor Author

adriangb commented Jan 23, 2023

Ok so I had a chance to look at your example and test in my example repo.

The issue with the solution you proposed is that you never install the transitive 3rd party dependencies (which may even be all 3rd party dependencies if there is no "root" pacakge/dependencies), so there's no layer that has "all of the things that are expensive to install but don't change often" which is what we want for good caching.

I updated my example repo to use this branch (adriangb/python-monorepo@0250dda) and tested that indeed doing poetry install --only app --no-path gives me transitive dependencies of app (in particular, app depends on lib and lib depends on pydantic and indeed pydantic gets installed).

So at least as proposed dependency groups are not an alternative to this PR.

@adriangb
Copy link
Contributor Author

pre-commit.ci autofix

@adriangb adriangb changed the title Add option to skip installing path dependencies Add option to skip installing directory dependencies Mar 6, 2023
@adriangb
Copy link
Contributor Author

adriangb commented Mar 6, 2023

@radoering I renamed path to directory as per your feedback in #6850

@adriangb
Copy link
Contributor Author

adriangb commented Mar 6, 2023

I think this will also be useful in CI workflows (so not Docker but similar) since the same principle of transitive deps changing less than source code apply.

@adriangb
Copy link
Contributor Author

adriangb commented Apr 5, 2023

@rodering, I addressed all of the feedback and got the branch in sync with the default branch. Apologies if the commits got messy, I'm working from plane wifi.

@adriangb adriangb requested a review from radoering April 5, 2023 16:11
@radoering radoering added area/installer Related to the dependency installer kind/feature Feature requests/implementations area/ci Related to CI/CD impact/changelog Requires a changelog entry impact/docs Contains or requires documentation changes area/cli Related to the command line and removed area/ci Related to CI/CD labels Apr 10, 2023
@github-actions
Copy link

github-actions bot commented Apr 10, 2023

Deploy preview for website ready!

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

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

@radoering radoering enabled auto-merge (squash) April 10, 2023 12:17
@radoering radoering merged commit 7c9a565 into python-poetry:master Apr 10, 2023
@adriangb adriangb deleted the skip-path-dep-install branch April 10, 2023 12:58
@adriangb
Copy link
Contributor Author

Thank you for pushing this across the finish line Randy!

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 <bot@walbeck.it>
Co-committed-by: renovate-bot <bot@walbeck.it>
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/cli Related to the command line area/installer Related to the dependency installer impact/changelog Requires a changelog entry impact/docs Contains or requires documentation changes kind/feature Feature requests/implementations
Projects
None yet
5 participants