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

Move to pip-compile-multi #786

Merged
merged 11 commits into from
Mar 1, 2022
Merged

Move to pip-compile-multi #786

merged 11 commits into from
Mar 1, 2022

Conversation

stevejalim
Copy link
Contributor

@stevejalim stevejalim commented Jan 24, 2022

This changeset swaps Basket form pip-tools to using pip-compile-multi as our chosen dependency helper, plus retains and evolves a Makefile-driven approach to using pip-compile-multi.

Notes:

  • UPDATED: this continues to compile requirements inside the Docker container

  • for now, while we generate separate requirements/prod.txt and requirements/dev.txt files, we continue to use the superset of dependencies (requirements/dev.txt) for building the single Docker image. Splitting Docker images is deferred to Refactor Docker setup to support separate dev/test and prod images, similar to Bedrock #784 to keep this change cleaner.

  • one dependency - the FuelSDK for Salesforce - had to be removed as it has a very stale dependency and patching the supply chain isn't worth it as FuelSDK isn't used in the Basket codebase any more, it appears.

To test

  • Run make compile-requirements
  • Run make compile-requirements again and confirm that the deps are rebuilt, but have no changes
  • Run make clean build test to ensure that full images still build
  • Check CI is happy

* Switch to pip-compile-multi, splitting out dev deps from prod deps

  It's worked fine, albeit with a TODO in the Dockerfile to actually build a separate dev-specific image,
  which will come in a separate commit.

  Went with the pattern of auto-installing the dependency-management tool via the makefile. Will likely
  switch this to an alert + install instructions

  This change was needed because in-container compilation was failing and various pip+pip-tools versions attempted clashed due to
  ```
  TypeError: make_requirement_preparer() got an unexpected keyword argument 'wheel_download_dir'
  ```
  which may have been a chicken-and-egg problem, but was not worth solving when a simpler route was around.

* Rebuild requirements on the host, not in the container, but retain Makefile commands as the primary
  invocation route - `make compile-requirements` and  `make upgrade-requirements`
…mpile-multi

Now, instead of force-intalling pip-compile-multi, we get a prompt showing us what do to (and so providing some flexibility)

Also note that some dev subdeps have been minorly bumped.

IMPORTANT: if you currently try to recompile deps results in a failure with long-unmaintained suds-jurko, so this will need swapping out. The current set of deps builds and passes tests, at least
No longer needed and was causing a dependency install issue due to its suds-jurko subdep
@stevejalim stevejalim requested a review from pmac January 24, 2022 16:48
@stevejalim stevejalim added the Needs review PR needs review label Jan 24, 2022
Invoke with:

   $ make check-requirements

and update hard-pinned dependencies as you wish
robhudson
robhudson previously approved these changes Jan 24, 2022
Copy link
Contributor

@robhudson robhudson left a comment

Choose a reason for hiding this comment

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

Just some questions, otherwise looks good. r+wc

Makefile Outdated Show resolved Hide resolved
bin/compile-requirements.sh Outdated Show resolved Hide resolved
docs/install.rst Outdated Show resolved Hide resolved
@robhudson
Copy link
Contributor

Hmm, this failed for me b/c I don't have mysql on my host machine:
OSError: mysql_config not found

This gives us guaranteed install stability regardless of the source platform being used to run the update

Note that pip-tools and pip-compile-multi are installed ONLY for compiling or upgrading requirements, rather than adding them to the main build.
pip-tools needed pinning to a recent version to get around incompatibility issues with pip 21 and pip-tools.
@stevejalim
Copy link
Contributor Author

stevejalim commented Jan 25, 2022

@robhudson @pmac One additional point is that make docs fails for me both on this branch and on master. Would one of you mind trying it to see if you can reproduce?
Error response from daemon: OCI runtime create failed: container_linux.go:380: starting container process caused: exec: "make": executable file not found in $PATH: unknown

The executable in question seems to be make, am thinking, based on this entry in docker-compose.yml:

docs: .make.docker.pull  
    ${DC} run --rm web make -C docs/ clean html

so, adding that in now and trying it out

UPDATE: installing make is causing issues with invalid signatures, but I just realised that one can simply go into /docs and run make clean html (which is what the main Makefile's make docs does anyway), so it's not a blocker as long as RTD is happy, too

@stevejalim stevejalim dismissed robhudson’s stale review January 25, 2022 11:10

Refactored, so re-requesting review

One can still compile the docs by using the Makefile in /docs and issuing

  $ make clean html
* hard-pin latest working combo of pip, pip-tools and pip-compile-multi
* add in custom header to make recompliation step unambiguous
* update Makefile: remove unneeded upgrade-requirements option; update help
* recompile reqs with minor bumps
Copy link
Contributor

@robhudson robhudson left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@stevejalim stevejalim removed the Needs review PR needs review label Mar 1, 2022
# -r requirements/prod.in
# pytz-deprecation-shim
# tzlocal
billiard==3.6.4.0 \
Copy link
Member

Choose a reason for hiding this comment

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

We should just double check that this version is compatible with our version of Celery.

Copy link
Member

@pmac pmac left a comment

Choose a reason for hiding this comment

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

Looks great! 🥇

@stevejalim stevejalim merged commit ca50f72 into master Mar 1, 2022
@stevejalim stevejalim deleted the 782-move-to-pip-compile-multi branch March 1, 2022 14:40
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.

3 participants