WorryFree Computers   »   [go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

verify pip dependencies in CI #22254

Merged
merged 2 commits into from
May 20, 2024
Merged

verify pip dependencies in CI #22254

merged 2 commits into from
May 20, 2024
+124 −18

Conversation

KevinMind
Copy link
Contributor
@KevinMind KevinMind commented May 17, 2024 •
edited

Description

Add check to CI verifying that pip packages are installed in the container.

  • add script to determine all dependencies specified in a set of requirements.txt files are installed via pip show
  • add pip_development stage to docker build to install dev related dependencies (like dennis previously in it's own requirements file) (not included in final image)
  • merge locale.txt into dev.txt dependencies (we have dev stage now, don't need it)
  • add ci check to verify pip. packages are installed on PRs

Context

We rely on some (mildly) complex logic to install our dependencies. For this reason, it makes sense to verify that dependencies that are "required" actually get installed in the final container. This PR adds such a check.

Having this check is very useful (for example) in this PR that wants to modify how dependencies are finally added to the image #22253

Testing

You can verify this check works, by adding a package to a requirements.txt file, NOT installing it, and running the check command specifying that file.

  1. add a package (any package) to prod.txt
  2. run make check
  3. should exit 1 that the package you added is "<package": Missing"

@KevinMind KevinMind requested a review from diox May 17, 2024 13:11
@diox
Copy link
Member
diox commented May 17, 2024

No deal breakers, but:

  • Because it's also printing packages that were found, it's a bit annoying to find the missing package(s) when that happens. Do we need to print the found packages ?
  • It's rather slow - around 25 to 30 seconds on my machine. Looks like because the individual calls to pip show are slow. Maybe we can address this ? It takes pip freeze -l under a second to list all installed packages...
  • Should we check for versions as well ?

@diox
Copy link
Member
diox commented May 17, 2024

It's rather slow - around 25 to 30 seconds on my machine. Looks like because the individual calls to pip show are slow. Maybe we can address this ? It takes pip freeze -l under a second to list all installed packages...

pip freeze --all --exclude-editable | sort -u is almost comparable to cat requirements/dev.txt requirements/prod.txt| grep -E -v '^\s*(#|--hash)' | cut -d ' ' -f 1 | sort -u using diff FYI. There are only 3 things to deal with using this approach:

  • The special package[foo] case
  • bitarray == 2.9.2 in requirements/prod.txt should be specified without spaces
  • Some packages are specified with underscores instead of dashes and should be fixed as well

As a bonus this approach would deal with the version numbers being different.

@KevinMind
Copy link
Contributor Author

Sweet. I'll add this.

diox
diox approved these changes May 20, 2024
View reviewed changes
Copy link
Member
@diox diox left a comment

Choose a reason for hiding this comment

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

I wonder if dependabot will recreate package names with underscore or capital letters... I guess we'll find out, we can easily adapt the script if it does.

(remove the TMP: from the PR title before merging)

@KevinMind KevinMind changed the title TMP: verify pip dependencies in CI verify pip dependencies in CI May 20, 2024
@KevinMind KevinMind merged commit 14fabcb into master May 20, 2024
14 checks passed
@KevinMind KevinMind deleted the test-deps branch May 20, 2024 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diox diox diox approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@KevinMind @diox