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

python: tidy dependency management #137

Merged
merged 9 commits into from
Nov 16, 2018
Merged

python: tidy dependency management #137

merged 9 commits into from
Nov 16, 2018

Conversation

mrpollo
Copy link
Contributor

@mrpollo mrpollo commented Nov 1, 2018

This commit introduces Pipenv into the workflow allowing for
deterministic releases due to dependency pinning

  • plus the unintended consequence of updating README documentation

@mrpollo mrpollo requested a review from bkueng November 1, 2018 17:56
Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

Pipenv looks nice, but I had a mixed experience so far. pipenv install took more 10 minutes to finish.

Can you add the pipenv shell to the usage section as well?

README.md Show resolved Hide resolved
Pipfile Outdated Show resolved Hide resolved
@mrpollo
Copy link
Contributor Author

mrpollo commented Nov 5, 2018

@bkueng updated docs

Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

Thanks for the update

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
This commit introduces Pipenv into the workflow allowing for
deterministic releases due to dependency pinning
The previous headings where getting out of hand when adding sub
headings, I also moved the build status badge below the main title.
@pietrodn
Copy link
Contributor

pietrodn commented Nov 12, 2018

Pipenv is great for deterministic builds, because it solves the long-time dilemma of whether to lock package versions in requirements.txt.

I think it should be used as the primary package management system for the project (i.e., for CI and documentation). Pipenv is also recommended by the official Python guide about application dependency management. So far, so good.

However, I also had mixed experiences in using Pipenv, especially on resource-constrained systems.
Here are the problems I encountered:

  1. Pipenv is a fairly large project with some dependencies, which must be installed globally. One may not wish to install it globally on the system; or worse, one may not have the permission to do so. pip is generally shipped within the Python distribution; if it's not, the builtin Python venv module ensures that it gets installed within any newly created virtual environment, without needing root privileges.
  2. As outlined by @bkueng, Pipenv is much slower than pip, because package locking takes a lot of time. Personally, some time ago I ditched Pipenv for pip+venv exactly because of this.
  3. Pipenv does not play well with virtual environments created with venv, which is the standard, suggested way of creating virtual environments in Python 3. Both venv and Pipenv are officially recommended by the Python documentation, so this is strange. But we know Python packaging is a mess, so I guess this is ok... sort of.
  4. I really don't like using pipenv run to execute my scripts, especially in automated, non-interactive scenarios. It slows down application startup, and it requires additional environment variables to be set (1, 2). When automating things (e.g. with systemd services or cron), you have to make sure that pipenv is on your $PATH and that the working directory is set correctly.
  5. By default, Pipenv creates the virtual environment in a non-deterministic folder name, so either you set another environment variable to override this behaviour, or you're stuck with pipenv run to run your script non-interactively.

I think that all these problems may be solved by generating a requirements.txt file (very easy, with Pipenv: pipenv lock --requirements), and by committing it.
In this way, the user can choose the workflow of choice:

  1. deterministic but slow builds with Pipenv
  2. or, minimalistic and fast installation with pip.

The only drawback is that requirements.txt needs to be changed every time Pipfile.lock changes, but Pipenv does all the hard work. :-)

This isn't intended as an attack on Kenneth Reitz's work. IMO, Pipenv is great for deterministic builds, but falls short on some use cases, and there are valid reasons why a user may deliberately decide not to use it in favor of a more traditional toolchain.

@pietrodn
Copy link
Contributor

I would also include macOS installation instructions (see #142).

@bkueng
Copy link
Member

bkueng commented Nov 13, 2018

Thanks for the extensive feedback @pietrodn! A combination with generating requirements.txt makes sense then. @mrpollo what do you think?

@mrpollo
Copy link
Contributor Author

mrpollo commented Nov 13, 2018

Thanks for the feedback @pietrodn, I'll add the requirements.txt and instructions on how to generate the file, we should try to add a test if we can figure out how, this will ensure we don't push a lock w/o an update to the txt file

@pietrodn
Copy link
Contributor

We could make Pipenv generate a new requirements.txt, run diff against the one present in the working directory, and check the exit code.

@mrpollo
Copy link
Contributor Author

mrpollo commented Nov 13, 2018

I think I addressed the comments in my recent commits, please review

@pietrodn
Copy link
Contributor

pietrodn commented Nov 14, 2018

Looks good!

I tried the installation with both methods (venv + pip and pipenv), both with cached dependency downloads.

For pip I used this script (install.sh):

#!/bin/bash

python3 -m venv venv
venv/bin/pip install -r requirements.txt

For pipenv, I used pipenv sync --python=3.7.

Here are the performances, measured with the time utility:

./install.sh              19,11s user 6,43s system 84% cpu 30,399 total
pipenv sync --python=3.7  36,55s user 9,82s system 196% cpu 23,573 total

Pipenv runs faster by exploiting multiple CPUs.
pipenv install takes approximately the same time.

After removing Pipfile.lock, however, pipenv install takes forever:

pipenv --python=3.7 install  65,45s user 25,37s system 7% cpu 21:15,45 total

Dependency locking is very time-consuming.

run_tests.sh Outdated Show resolved Hide resolved
@mrpollo
Copy link
Contributor Author

mrpollo commented Nov 15, 2018

thanks for the metrics @pietrodn, I think we should always provide a lock if we are to move forward with this workflow change to pipenv.

My next goal is to make this a proper python package and expose flight_review as a command line tool to generate html reports out of ulog, I'll open an issue to discuss

@bkueng bkueng merged commit a9344f8 into master Nov 16, 2018
@bkueng bkueng deleted the rroche-tidy-deps branch November 16, 2018 08:19
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