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 'Usecase 4' for poetry+tox #7346

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

sirosen
Copy link

@sirosen sirosen commented Jan 12, 2023

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.


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)

@fetzerch
Copy link

fetzerch commented Jan 26, 2023

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 poetry run in commands and an explanation why that's typically not correct would be really helpful.

@sirosen
Copy link
Author

sirosen commented Jan 26, 2023

As the author, I agree.
I think the docs really ought to describe what types of usages the different usages support, not just number them 1, 2, 3.
And I'd love to make more incremental improvements to the doc here.

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.

Copy link
Member

@finswimmer finswimmer left a 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

@sirosen
Copy link
Author

sirosen commented Jan 27, 2023

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.

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.
e.g. "Usecase 2, typical for services and applications"

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!

Poetry will detect that it is running in the venv created by tox and will not activate any other venv.

That sounds like I did not understand that usage correctly. Good to know!

Even as a somewhat experienced user of these tools, poetry run looked to me like an explicit invocation back into the Poetry-managed venv. I think we should update the doc as you suggest to remove it, but I'll do a future PR for that.

I would like to change usecase 2 by adding this...

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.
@sirosen
Copy link
Author

sirosen commented Jan 27, 2023

I just rebased and pushed, and was about to walk away, but I noticed that I misread your previous comment.

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.

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 install_command?

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.
@sirosen sirosen changed the title Modify 'Usecase 2' for poetry+tox Add 'Usecase 4' for poetry+tox and update 'Usecase 2' Jan 27, 2023
@sirosen
Copy link
Author

sirosen commented Jan 27, 2023

I just tested applying the install_command change to one of my projects and it appears to me that {packages} is being expanded to include dependencies from the project. I'm coming back here to flag it right away because I'm somewhat concerned that I've written that up in a misleading way.

@sirosen
Copy link
Author

sirosen commented Jan 27, 2023

It looks to me like tox currently extracts dependencies from the wheel or sdist build and always appends them to the install command in the install_package_deps stage.
If I set install_command = foo, then the install_package_deps stage will show foo 'requests<3.0,>=2.0' for a project requiring requests.

My attempts to trace it out (as someone who has not worked very much on the tox codebase) led me here:

https://github.com/tox-dev/tox/blob/acadf36d08c5732e36e1547ba716d2c094c525e7/src/tox/tox_env/python/virtual_env/package/cmd_builder.py#L100

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 install_package_deps and that step can't be skipped.

--no-deps has no effect because pip is not doing the resolution of primary package dependencies (though it will of course be involved in resolving dependency chains).

I'm removing the addition of install_command. It can be done in separate work if someone can trace this out more thoroughly. For now, I'm satisfied to get proper --sync --with ... usage demonstrated.
And later I can update these examples to use wheel builds, since that's a surprisingly significant speed improvement.

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.
@sirosen sirosen changed the title Add 'Usecase 4' for poetry+tox and update 'Usecase 2' Add 'Usecase 4' for poetry+tox Jan 27, 2023
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
Copy link
Member

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

Copy link
Author

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.

Copy link
Author

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
Comment on lines 117 to 122
[testenv]
allowlist_externals = poetry
commands_pre =
poetry install --no-root --sync --with test
commands =
pytest tests/
Copy link
Member

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

Copy link
Author

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!

Copy link
Author

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 Show resolved Hide resolved
docs/faq.md Outdated
Comment on lines 147 to 151
`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.

Copy link
Member

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.

Copy link
Author

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?

sirosen and others added 2 commits January 31, 2023 17:31
- rename usecase 4
- put the pytest run in a dedicated testenv
- skip install and don't no-root the mypy testenv
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants