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

build: make use of poetry dependency groups #152

Merged
merged 9 commits into from
Oct 28, 2022
Merged

build: make use of poetry dependency groups #152

merged 9 commits into from
Oct 28, 2022

Conversation

maxrake
Copy link
Contributor

@maxrake maxrake commented Oct 19, 2022

Poetry 1.2+ allows for dependency groups. Three new groups were created to split up what used to be considered the dev dependencies. All three groups were made to be optional and will therefore require specifying during a poetry install command with the --with option. The test group includes packages needed to run the test suite. The ci group includes packages that are only used in the Continuous Integration (CI) environment, in GitHub workflows. The qa group includes (or will include, when there are more) packages related to ensuring Quality Assurance (QA) (e.g., with linting, formatting, and typing tools).

Additional actions taken:

  • Ensure tox environments have test dependency group installed
  • Use the minimum dependency groups necessary in CI workflows
  • Bump poetry.lock dependencies
    • The lockfile was regenerated and updated to match the latest install of poetry and the updated pyproject.toml file
    • An issue was found with the casing of the levenshtein package
      • It would not complete Phylum processing due to a mis-match in name: it expects Levenshtein
      • Even though the name is controlled by poetry, the lockfile was manually updated to change the case on this package to enable analysis completion
      • The issue has been brought forward to the relevant Phylum developers and will be addressed in a separate issue(s)
  • Use poetry plugin poetry-plugin-export for export command
    • The export command is now provided as a plugin
      • It is installed by default for the poetry v1.2 release, but future releases will deprecate this automatic install
    • Installing the plugin directly now does not harm the function of the plugin or cause any errors given that it may already be installed
    • This change is getting ahead of the future deprecation
  • Update CONTRIBUTING.md to account for poetry dependency groups

Closes #15

Checklist

  • Does this PR have an associated issue (i.e., closes #<issueNum> in description above)?
  • Have you ensured that you have met the expected acceptance criteria?
  • Have you created sufficient tests?
    • The auto_updates and test workflows were exercised manually with the changes here and shown to work as expected
    • The preview and release workflows have essentially the same changes and will be verified when this branch is merged to main and included in a release, respectively
  • Have you updated all affected documentation?

Poetry 1.2+ allows for dependency groups now. Three new groups were created to split up what used to be considered the `dev` dependencies. All three groups were made to be optional and will therefore require specifying during a `poetry install` command with the `--with` option. The `test` group includes packages needed to run the test suite. The `ci` group includes packages that are only used in the Continuous Integration (CI) environment, in GitHub workflows. The `qa` group includes (or will include, when there are more) packages related to ensuring Quality Assurance (QA) (e.g., with linting, formatting, and typing tools).
The lockfile was regenerated and updated to match the latest install of `poetry` and the updated `pyproject.toml` file.
The `export` command is now provided as a plugin. It is installed by default for the `poetry` v1.2 release, but future releases will deprecate this automatic install. Installing the plugin directly now does not harm the function of the plugin or cause any errors given that it may already be installed. This change is getting ahead of the future deprecation.
@maxrake maxrake requested a review from a team as a code owner October 19, 2022 18:37
@maxrake maxrake self-assigned this Oct 19, 2022
@maxrake maxrake requested a review from cd-work October 19, 2022 18:37
@github-actions
Copy link

Phylum OSS Supply Chain Risk Analysis - SUCCESS

The Phylum risk analysis is complete and did not identify any issues.

View this project in the Phylum UI

Copy link

@cd-work cd-work left a comment

Choose a reason for hiding this comment

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

Is test/ci/qa just supersets/subsets of each other? I don't understand why one would want to make their testing setup so much more complicated for what seems to be like no benefit?

What do you gain other than saving 3kb of network traffic when running your CI?

.github/workflows/auto_updates.yml Outdated Show resolved Hide resolved
It turns out that the project does not need to be installed in order for `poetry` to update the lockfile. Removing this step saves some workflow execution time.
@maxrake
Copy link
Contributor Author

maxrake commented Oct 28, 2022

Is test/ci/qa just supersets/subsets of each other? I don't understand why one would want to make their testing setup so much more complicated for what seems to be like no benefit?

What do you gain other than saving 3kb of network traffic when running your CI?

The release notes for poetry 1.2 has a section on dependency groups that might help to explain some of this:

Think of dependency groups as labels associated with your dependencies: as all groups will be installed by default, they are simply a way to organize the dependencies logically.

This PR makes the three new groups that were previously part of "dev-dependencies" optional so they will NOT be installed by default.

Dependency groups, other than the implicit main group, should generally contain additional dependencies that are part of your development process, as installing them is only possible using Poetry and poetry install.

Agreed...which is how these are setup.

As the dev-dependencies is now deprecated, projects should migrate to the new group syntax as soon as possible.

Since the transition to groups (and the use of new --with type options to specify them) is already necessary, the thought here was to create logical groups that could be installed only when needed. True, the network traffic is not much, but limiting the download and install times to the minimum for each use case does help to save workflow execution time. It will likely be more of an obvious savings once the Quality Assurance issue (#14) is complete since that will likely add another dozen first level deps to the qa group...with an untold number of secondary deps.

@maxrake maxrake requested a review from cd-work October 28, 2022 16:25
Copy link

@cd-work cd-work left a comment

Choose a reason for hiding this comment

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

This system definitely seems overly granular, though I suppose that's mostly a complaint against poetry and not this PR. So this should be good to go.

@maxrake maxrake merged commit ccc0d76 into main Oct 28, 2022
@maxrake maxrake deleted the beat_poetry branch October 28, 2022 20:04
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.

Make use of poetry 1.2+ groups
2 participants