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

Support installing Poetry dependency groups #1080

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

Conversation

Niicck
Copy link

@Niicck Niicck commented Feb 7, 2023

Closes #663
Closes #977
Closes #873

This PR adds a install_groups method to nox_poetry Session proxy. It has the same interface as session.install except that you pass in the dependency groups that you want to install. This feature requires poetry >=1.2.0, but the rest of the code is compatible with <1.2.0, there are no breaking changes.

Example usage: session.install_groups("lint", "test")

Implementation Notes

  • install_groups uses poetry export's --only flag to just get the requirements for the groups you specify. They are then installed via pip install -r.
  • Creating install_groups as a new method on the Session class was the cleanest way to implement this feature. My first approach followed this suggestion of adding poetry_groups as a kwarg of install. But this created confusing interactions between installing individual dependencies and installing entire group dependencies. The initial support PR had the right approach, so I sprang off from there.
  • I had initially dropped python3.7 and poetry <1.2.0 support, but I re-added it since we still want to support them.
  • The filename generated by _PoetrySession.export_requirements is now more explicit. If we're generating a constraints file to only install individual packages, then export_requirements will generate a constraints.txt. If we're creating a requirements file that is intended to install the entirety of select poetry dependency groups, then export_requirements will generate a requirements file with the format group1,group2-requirements.txt. The requirements.txt file is prefixed with the groups so that if our user inputted install_groups change, we'll regenerate the requirements.txt file, even if the poetry.lock hash hasn't changed.
  • In order to make our functional tests faster, I only added one session that uses poetry v1.2.0. We could add more test session combinations if that would be desirable.

@samypr100
Copy link

@Niicck I think this also closes #873 due to the changes in src/nox_poetry/poetry.py unless I'm mistaken

@samypr100
Copy link

samypr100 commented Feb 8, 2023

@Niicck from importlib import metadata is python 3.8+ only, but since importlib-metadata is a dependency (from nox) its possible to use import importlib_metadata as metadata on 3.7 and below as a replacement that way we can keep 3.7 compatibility as stated in the docs for this project

e.g.

try:
    from importlib import metadata
except ImportError:
    import importlib_metadata as metadata

Edit: Only version v3.x+ of importlib-metadata has the py.typed, if you get issues w/ mypy on 3.7 # type: ignore's might be needed if the importlib-metadata package is not high enough (via the dev dependencies which resolves to 1.7.0)

@Niicck
Copy link
Author

Niicck commented Feb 8, 2023

@samypr100 Big thanks for that check. Tests and mypy are now passing for 3.7

@johnthagen
Copy link

johnthagen commented Feb 12, 2023

Edit: There is now an MR for this:


As a slight tangent, when I was playing around with --with and --only I noticed this Poetry bug:

If anyone was intrepid, fixing that upstream would make using this nox-poetry feature less silently error prone to typos.

@johnthagen
Copy link

@cjolowicz Would you be able to review this MR?

@Niicck
Copy link
Author

Niicck commented Mar 22, 2023

Hello friends. I'm going to be offline for the next couple of months. If there are change requests in future reviews, someone else is welcome to address them and add to this PR.

In the meantime, if people want to install poetry groups using nox, I have a workaround install_poetry_groups function that I use with vanilla nox. It's not unit-tested and it's not as robust as this PR's implementation, but it's functional enough.

Here's the code for that helper function with a couple examples:

def install_poetry_groups(session, *groups: str) -> None:
    """Install dependencies from poetry groups.

    Using this as s workaround until my PR is merged in:
    https://github.com/cjolowicz/nox-poetry/pull/1080
    """
    with tempfile.NamedTemporaryFile() as requirements:
        session.run(
            "poetry",
            "export",
            *[f"--only={group}" for group in groups],
            "--format=requirements.txt",
            "--without-hashes",
            f"--output={requirements.name}",
            external=True,
        )
        session.install("-r", requirements.name)

@nox.session(python=PYTHON_VERSIONS)
@nox.parametrize("django_version", DJANGO_VERSIONS)
def test(session: nox.Session, django_version: str) -> None:
    """Run the pytest suite."""
    args = session.posargs
    install_poetry_groups(session, "main", "test", "dev", "coverage")
    session.install(f"django=={django_version}")
    try:
        session.run("coverage", "run", "--parallel", "-m", "pytest", *args)
    finally:
        if session.interactive:
            session.notify("coverage", posargs=[])

@nox.session(python=PYTHON_VERSIONS[0])
def coverage(session: nox.Session) -> None:
    """Produce the coverage report.

    Combines the results from all test runs from all versions of python. This is because
    some logic branches may only apply to certain versions of python - we want to avoid
    false negative coverage failures when those branches aren't covered by pytest runs
    from other python versions.
    """
    args = session.posargs or ["report"]
    install_poetry_groups(session, "coverage")

    if not session.posargs and any(Path().glob(".coverage.*")):
        session.run("coverage", "combine")

    session.run("coverage", *args)

Comment on lines +119 to +124
if groups:
args.extend(f"--only={group}" for group in groups)
elif self.config.is_compatible_with_group_deps():
args.append("--with=dev")
else:
args.append("--dev")
Copy link

Choose a reason for hiding this comment

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

Why is the check on self.config.is_compatible_with_group_deps() only for --with? Isn't --only also only available for poetry>=1.2?

Copy link
Author

Choose a reason for hiding this comment

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

What we want to do is export the "main" group along with the "dev" group. poetry export --with=dev implicitly exports the "main" group as well.

If we ran poetry export --only=dev then we'd only be exporting the "dev" group without "main," which is not what we want.

poetry export --with=dev is the equivalent of poetry export --dev. We want the expected default behavior to be the same regardless of what version of poetry the user is using.

Copy link
Author

Choose a reason for hiding this comment

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

Reading this again, it looks like I misunderstood your comment. Ok. You're asking why the first if groups: clause doesn't also check for self.config.is_compatible_with_group_deps() since that if groups: clause uses the --only flag which is only available for poetry v>=1.2.

The reason is because earlier in the code we have a check within _PoetrySession.export_requirements that makes sure that if you're using "groups" that you're using the right version of poetry. And there's a unit test to ensure that.

Copy link

Choose a reason for hiding this comment

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

Got it. Thanks!

@RomainBrault
Copy link

Any hope to merge this soon @cjolowicz ?

Copy link

@bernardcooke53 bernardcooke53 left a comment

Choose a reason for hiding this comment

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

Came looking to make this PR and found it was already here 🎉

@johnthagen
Copy link

@cjolowicz Curious, is nox-poetry something you'd like to continue to maintain, or are you using helper scripts like what you've mentioned before (#663 (comment)). Would you like to pass nox-poetry off to others in the community who'd like to continue to move it forward?

@RomainBrault
Copy link

The minimum should be avoiding warnings (#873) and anticipate future breakage(s).

@cjolowicz
Copy link
Owner

@cjolowicz Curious, is nox-poetry something you'd like to continue to maintain, or are you using helper scripts like what you've mentioned before (#663 (comment)). Would you like to pass nox-poetry off to others in the community who'd like to continue to move it forward?

@johnthagen I've been swamped with other work, but I expect to circle back to this project in the coming few weeks. I'll do my best to come up with a sustainable model for maintaining this package going forward. That might involve passing it on to others or reducing the bus factor.

@johnthagen
Copy link

@Niicck FYI, there are some merge conflicts in this branch now, likely from

@johnthagen
Copy link

@Niicck I was curious if you still had any interest in getting this branch back ready for a merge. Would ❤️ this feature.

@Niicck
Copy link
Author

Niicck commented Oct 4, 2023

@Niicck I was curious if you still had any interest in getting this branch back ready for a merge. Would ❤️ this feature.

If @cjolowicz is interested in approving this PR/feature, then yes I'd be happy to bring the branch back into merge shape. If not, then I won't put any more development time into something that we're not intending to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants