IN 1240 - Replace pipenv check with pip-audit and maintenance #206
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Why these changes are being introduced: As of pipenv 2025.0.1 the use of `pipenv check` would throw an error, indicating that the library `safety` was not installed. It worked to run `pipenv check --auto-install` which would temporarily install `safety`, but this was not ideal for multiple reasons. First, we anticipate potentially moving away from `pipenv`. Second, it appears that `safety` is moving to a pay / subscription model. Third, it remains a little obfuscated what `pipenv check` is actually doing. As this new situation affects all builds in Github Actions CI, we need a way to scan for vulnerabilities that ideally is not a massive overhaul of our vulnerability scanning approach. How this addresses that need: `pip-audit` is a nice standalone, open-source library that performs very similar work to `safety`. This commit replaces `pipenv check` (which was `safety` under the hood) with `pip-audit`. NOTE: until we can install a non-pinned version of luigi, we must ignore vulnerability PYSEC-2024-159 (which should not affect us). Side effects of this change: * Builds will be successful in Github Actions Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-1240
Why these changes are being introduced: This CLI was stuck on python 3.11 for awhile while untangling some mypy linting errors. Turns out it was luigi >= 3.5.2 that presented the issues. With luigi still pinned, we can perform python version update and update dependencies. How this addresses that need: * Python version bumped to 3.12 * Luigi remains pinned, for now, at 3.5.1 * Dependencies updated Side effects of this change: * Python version updated Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-1240 * https://mitlibraries.atlassian.net/browse/IN-1258
Why these changes are being introduced: luigi=3.5.2 introduced a litany of type checking errors for our usage. As such, we had luigi pinned to 3.5.1 for some time. With the introduction of pip-audit, and a found vulnerability for 3.5.1, and bumping python to 3.12, it was time to finally address this. We fairly heavily extend luigi.Task and luigi.LocalTarget, and use those base extensions throughoug the project. Because luigi is poorly typed, even with the `luigi.mypy` module, we had cascading type check errors. Manually creating .pyi stubs was explored -- and it works -- but the benefit did not seem to outweigh the cost of creating and maintaing them. Particularly since most of this project is luigi task boilerplate; each task, e.g. ExtractDWEmployees, is basically just class definitions with a minimal amount of actual business logic. This was the appeal of luigi, and it's why these .pyi files would be nearly doubling the codebase. The ultimate solution feels to just skip type checking for luigi based classes, given a) luigi is poorly typed already, and b) the signatures are effectively the same for each task. IDE's will still benefit from understanding what classes can do, but we skip linting errors. How this addresses that need: * Unpins luigi, resulting in install of luigi=3.6.0 at time of this commit * Adds explicit type checking ignores for hrqb.base.* and hrqb.task.* * Remove vulnerability ignore now no longer present in luigi Side effects of this change: * Luigi is updated, but type checking is lost for most luigi tasks (at least temporarily) Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-1258
ehanson8
approved these changes
May 13, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose and background context
This PR accomplishses a couple of tightly coupled things:
pipenv checkto usepip-audit, which is applied broadly across repos (Jira ticket)luigiresulting in installation of3.6.0, and some necessary type checking skipsluigihas been pinned to3.5.1for some time to avoid a barrage of type checking errors that3.5.2introduced. But with the move topip-audit, a vulnerability was found with3.5.1that version3.6.0(most recent) resolves. While the upgrade to python 3.12 was inconsequential, upgradingluigiresurfaced those mypy type checking errors.There is a good deal of commentary in the commit that unpins
luigi, but a short paraphrasing here:This project is largely a wrapper around
luigitasks. We define some custom classes that extendluigi.Taskandluigi.LocalTarget(the output of tasks). Each of our actual tasks is just an extension of these; 99% boilerplate with a sprinkling of business logic. This is what madeluigiappealing!However,
luigiis very poorly typed. Even the optionalluigi.mypymypy plugin does not seem to help (possibly an artifact of our extensions. Therefore it was decided to skip mypy type checking forhrqb.base.*andhrqb.task.*. In doing so, we:luigiwhich is important for vulnerability reasons, and keeping up-to-dateAs a big fan of type annotations and linting, the decision was not made lightly. But this allows us to keep this repository up-to-date dependency wise, and as the git commit mentions, there are some options if we'd like to layer on type checking again in the future.
How can a reviewer manually see the effects of these changes?
make test+make lintis about our best option at the moment, given the sensitivity of the data.Includes new or updated dependencies?
YES: bumps to python 3.12 and unpins
luigiresulting in installation of3.6.0Changes expectations for external applications?
NO
What are the relevant tickets?
Developer
Code Reviewer(s)