-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 'Usecase 4' for poetry+tox #7346
base: main
Are you sure you want to change the base?
Conversation
I just found this PR, when trying to understand how to best use Poetry & Tox. The current documentation is imho really a bit vague and not exactly easy to understand. Your PR improves it quite a bit. My usecase is testing an application that users would install from pypi/pip. The testing and linting should happen in an environment with locked dependencies, so that developers don't get 'new' linter warnings in random PRs, just because there's a new release of pylint. Without having read your PR description, I wouldn't really know which usecase of the examples matches mine. Therefore - if the project maintainers agree - I would recommend to extend the usecase description. Even the points from your bullet list would be a valuable extension of the documentation. You often find examples that suggest using |
As the author, I agree. I tried to stick with the existing doc format and level of brevity even though it is, IMO, to the detriment of the doc, to give this the best possible chance of merging quickly. Although I'm happy to make changes, I first want to get some confirmation that I can get doc changes through the PR pipeline. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @sirosen,
thanks a lot for your contribution. I really appreciate it 👍
The current FAQ is focused on showing how to install what in the venv created by tox
and who is responsible for resolving the dependencies. I think we should keep this focus.
Nevertheless I think it is a good idea to show a more complex config with multiple environments and the usage of Poetry's dependency groups as well, but in addition to the existing usecases.
it does not run commands with poetry run, which looks correct but actually means that the commands are executing from the poetry environment, rather than the tox environment
You are correct, that poetry run
can be removed from the command, because Poetry will detect that it is running in the venv created by tox
and will not activate any other venv.
I would like to change usecase 2 by adding this to the config:
install_command =
python -m pip install --no-deps {opts} {packages}
Now tox
will build and install the project but will not install any dependencies.
fin swimmer
Understood. Even so I think it would help to flag these in some way to give a reader some handle on what use-cases the different usages match well. If I can work something out that doesn't seem too radical, I may want to submit a separate PR. Let's set that aside for this one though!
That sounds like I did not understand that usage correctly. Good to know! Even as a somewhat experienced user of these tools,
I'll make this change right now; that's an excellent suggestion! We're using this method at work and I've noticed that extra overhead but hadn't thought to do anything about it. |
This is a longer config, but it better demonstrates how `poetry install --no-root --sync` can be used to sync locked dependencies to tox environments. Importantly, it demonstrates the following improvements over the existing example: - it does not run commands with `poetry run`, which *looks* correct but actually means that the `commands` are executing from the poetry environment, rather than the tox environment - it shows the application of dependency groups to tox environments - it shows differentiated use of `skip_install` (and thereby encourages correct usage in concert with `mypy` in particular, which many projects struggle to handle) The main downside of this new example is that it is longer and more complex for the very simple cases. However, if we take "Usecase 2" as describing an application (rather than a library) which wants a developer experience in which locked dependencies are used for linting and test steps, then this is a more complete and realistic sample config.
As suggested by @finswimmer, use `install_command = ...` to pass `--no-deps` to pip in the second tox usage example. This skips an unnecessary step in which pip installs dependencies which poetry will then handle in the sync step. Additionally, normalize the style of commands a bit in the proposed update to Usecase 2. Commands are no longer inlined with the config key, which makes the example slightly longer, but also more consistent with the other examples.
49e85ab
to
0e358f1
Compare
I just rebased and pushed, and was about to walk away, but I noticed that I misread your previous comment.
I think you're saying you want the usage example I've proposed to be renamed as "Usecase 4", and for the pre-existing case 2 to be preserved, but with the addition of the At the very least, that's how I'm reading this right now, so I'm going to apply that change. I can always revert it if I've gotten this wrong; please just let me know. |
Restore the earlier 'Usecase 2', but with the install_command added. Move the new example to be 'Usecase 4', which is written as before.
I just tested applying the |
It looks to me like My attempts to trace it out (as someone who has not worked very much on the tox codebase) led me here: If I'm following it correctly (and there's no promise of that), the above is always done as part of building the install_command for
I'm removing the addition of |
Setting an `install_command` which runs with `--no-deps` does not demonstrate the desired behavior because tox is resolving first-order dependencies and appending them to the install command.
docs/faq.md
Outdated
@@ -109,6 +109,46 @@ commands = | |||
`tox` will not do any install. Poetry installs all the dependencies and the current package an editable mode. | |||
Thus, tests are running against the local files and not the built and installed package. | |||
|
|||
#### Usecase #4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to call it something like "Complex example".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree pretty strongly, but I understand where you're coming from, in that it is more complex than the others.
tox
can't use dependency groups, and I suspect we'd agree that dependency groups in poetry provide a better developer experience than extras. (Not to mention the issue that a published package exposes its extras.) Users of tox
are trying to do multi-tool and multi-version testing. My experience has been that their use-cases are generally much closer to this than the other ones. I could argue for calling it "Realistic example" (but I think that's a bad name and not productive for our conversation 😁 ).
Having laid out that reasoning, would you still like it renamed? I'm willing to do it, but wanted to air the counterpoint first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having tinkered with this again and read the suggestion below of an added pyproject.toml
, my perspective is moving away from my previous comment a little. I still think this is the best and most complete usage of tox+poetry we can present, but I definitely can acknowledge that it is more complicated than the other examples.
I renamed to Complex Usage with Dependency Groups
for now.
docs/faq.md
Outdated
[testenv] | ||
allowlist_externals = poetry | ||
commands_pre = | ||
poetry install --no-root --sync --with test | ||
commands = | ||
pytest tests/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any config made in the [testenv]
is inherited to all other tox environment. Unless overwritten there, it will be executed there in the same way. For users that are not aware of this, it can lead to surprising behavior.
This is why I would prefer to have an own environment definition for tests as well, e.g. [testenv:pytest]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I can get behind that, and I've seen plenty of projects write their configs that way for that reason. I'll make the update!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this with testenv:test
because I was trying to match the environment names to the dependency groups (on purpose), and to avoid putting tool names in there.
I'm happy to tweak it to testenv:pytest
if you strongly prefer though, no strong opinion from me.
docs/faq.md
Outdated
`poetry` is used to sync package dependencies and named dependency groups into | ||
the `tox`-created virtual environments. This allows dependency groups to provide | ||
dependencies for different `tox` environments, providing `test` to the main | ||
testenv, `style` to the `style` env, and `typing` to the `typing` env. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should show the pyproject.toml
here as well. You are consequently use the --with
parameter for the groups, which means that these groups must be marked as optional.
I guess it would be nice to have some groups optional and some not. So we can show the difference between --only
and --with
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of showing the pyproject.toml
, and that would make me rethink what I said about calling this one a "Complex Example". If we add the pyproject file, it becomes much more complex to read than the others (but also more informative).
Can we split the difference by calling it "complex", omitting pyproject.toml
for now, and I can try to add it in a follow-up PR?
- rename usecase 4 - put the pytest run in a dedicated testenv - skip install and don't no-root the mypy testenv
This is a longer config, but it better demonstrates how
poetry install --no-root --sync
can be used to sync locked dependencies to tox environments.Importantly, it demonstrates the following improvements over the existing example:
poetry run
, which looks correct but actually means that thecommands
are executing from the poetry environment, rather than the tox environmentskip_install
(and thereby encourages correct usage in concert withmypy
in particular, which many projects struggle to handle)The main downside of this new example is that it is longer and more complex for the very simple cases.
However, if we take "Usecase 2" as describing an application (rather than a library) which wants a developer experience in which locked dependencies are used for linting and test steps, then this is a more complete and realistic sample config.
I considered adding another example to make this separate from the existing one, but I think this is a better configuration which addresses the same basic cases as the existing example.
This is related to past issues and discussions in which users have wanted groups re-exposed as extras as an interface for
tox
. (e.g. #4842)