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

[MRG/REVIEW] Support Pipfile / Pipfile.lock with pipenv #649

Merged
merged 53 commits into from Jun 26, 2019

Conversation

@consideRatio
Copy link
Collaborator

consideRatio commented Apr 20, 2019

To help you review this PR

I've added a new buildpack, it will allow repo2docker to detect Pipfile / Pipfile.lock and use them to install Python packages with pipenv. Here is a repo that made me want this functionality.

There are 48 files changed in this PR, but most of them are relating to tests of the functionality added. Other than these tests, we have changes to...

.gitignore - When running tests locally I ended up with some artifacts, this change helped me avoid these.
.travis.yml - I declared the need for another build job for our CI to run tests against this buildpack.
docs/source/config_files.rst - I documented the new build pack
repo2docker/app.py - I added an import statement, sorted the imports alphabetically, and added the buildpack in the ordered list that decides what buildpack has priority assuming it decides it can do something through the detect function a buildpack should have.
repo2docker/buildpacks/init.py - Allow repo2docker.buildpacks module expose the new buildpack.
repo2docker/buildpacks/pipfile/init.py - The logic of the buildpack, this is the key part to review I think.

The rest of the files are for the tests within the tests/pipfile folder. I added quite a bit of tests I'd say!


About

This PR aims to close #174. The goal is to make repo2docker consider Pipfile and Pipfile.lock dependency files using pipenv.

Code strategy

The strategy adopted was to mimic sections for how a requirements.txt configuration file was treated but give these new Pipfile.lock or Pipfile files a higher precedence.

Documented process

I wrote in this discourse topic as I was speaking to a rubber duck to help myself think clearly along the way.

What to review

The files

  • Some documentation within docs/source/config_files.rst.
  • The actual functionality introduced all resides in repo2docker/buildpacks/python/__init__.py. All three functions are modified a bit, namely detect(), python_version(), and get_assemble_scripts().
  • Loads of added tests that can give clarity of what I wanted to accomplish in /tests/venv/pipfile.
  • Small additions to a .gitignore file catching some scrap generated after running all the pytest.

Concerns

  • Not using PyEnv that is streamlined to work alongside Pipenv.

    • Not using PyEnv to manage the Python version concerns me, I choose to parse Pipfile.lock (JSON), Pipfile (TOML), and runtime.txt with Regex within python_version() that is overriding the CondaBuildPack's function and influences the installed Python version.
    • Not using PyEnv, we ignore patch versions of Python, just like runtime.txt does, but the Pipfile format supports python_full_version specifications.
  • Not entering pipenv shell, missing out on automatic loading of .env files for example.

  • Not managing to install dependencies in a Pipfile with Python 3.5 or lower.

    Step 40/47 : RUN ${KERNEL_PYTHON_PREFIX}/bin/pipenv lock --python >${KERNEL_PYTHON_PREFIX}/bin/python
    ---> Running in c7ae30385795
    Creating a virtualenv for this project…
    Pipfile: /home/erik/Pipfile
    Using /srv/conda/bin/python (3.5.5) to create virtualenv…
    ⠙ Creating virtual environment...Already using interpreter /srv/conda/bin/python
    Using base prefix '/srv/conda'
    New python executable in /home/erik/.local/share/virtualenvs/erik-zof0I2Qp/bin/python
    ERROR: The executable /home/erik/.local/share/virtualenvs/erik-zof0I2Qp/bin/python is not >functioning
    ERROR: It thinks sys.prefix is '/home/erik' (should be '/home/erik/.local/share/virtualenvs/erik->zof0I2Qp')
    ERROR: virtualenv is not compatible with this system or executable
    
    ✘ Failed creating virtual environment 
    [pipenv.exceptions.VirtualenvCreationException]:   File "/srv/conda/lib/python3.5/site->packages/pipenv/vendor/click/decorators.py", line 17, in new_func
    [pipenv.exceptions.VirtualenvCreationException]:       return f(get_current_context(), *args, **kwargs)
    [pipenv.exceptions.VirtualenvCreationException]:   File "/srv/conda/lib/python3.5/site->packages/pipenv/cli/command.py", line 319, in lock
    [pipenv.exceptions.VirtualenvCreationException]:       ensure_project(three=state.three, >python=state.python, pypi_mirror=state.pypi_mirror)
    [pipenv.exceptions.VirtualenvCreationException]:   File "/srv/conda/lib/python3.5/site->packages/pipenv/core.py", line 574, in ensure_project
    [pipenv.exceptions.VirtualenvCreationException]:       pypi_mirror=pypi_mirror,
    [pipenv.exceptions.VirtualenvCreationException]:   File "/srv/conda/lib/python3.5/site->packages/pipenv/core.py", line 506, in ensure_virtualenv
    [pipenv.exceptions.VirtualenvCreationException]:       python=python, site_packages=site_packages, >pypi_mirror=pypi_mirror
    [pipenv.exceptions.VirtualenvCreationException]:   File "/srv/conda/lib/python3.5/site->packages/pipenv/core.py", line 935, in do_create_virtualenv
    [pipenv.exceptions.VirtualenvCreationException]:       extra=[crayons.blue("{0}".format(c.err)),]
    [pipenv.exceptions.VirtualenvCreationException]: /home/erik/.local/share/virtualenvs/erik->zof0I2Qp/bin/python: error while loading shared libraries: libpython3.5m.so.1.0: cannot open shared >object file: No such file or directory
    
    Failed to create virtual environment.
    

Opinionated

  • pipenv will be used to do the installation as compared to using it to create a requirements.txt file and then do the installation with pip.
  • The first among environment.yml, Pipfile.lock, Pipfile, and requirements.txt will be used to install packages.
  • The first among Pipfile.lock, Pipfile, and runtime.txt will be used to decide python_version().
  • setup.py is ignored if a Pipfile(.lock) exists, as they can declare such installations. This can for example be seen in this repo.

Why didn't I use PyEnv?

  • At the point pipenv install would run that in turn triggers PyEnv to setup the right Python version, a Python version would have already been installed. So using PyEnv would have required deeper changes that I wasn't ready to tackle without better understanding about repo2docker.
  • Installing PyEnv wasn't something I could do with pip or similar, but it required adding apt-get packages etc. first which was a hassle.
@meeseeksmachine

This comment has been minimized.

Copy link

meeseeksmachine commented Apr 20, 2019

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/an-unfolding-story-of-my-first-contribution-to-repo2docker/839/1

@consideRatio consideRatio changed the title [WIP] Support Pipenv [WIP] Support Pipfile / Pipfile.lock with pipenv Apr 21, 2019
consideRatio added 10 commits Apr 21, 2019
As setup.py can be explicitly installed or not from a Pipfile I reasoned
that it should not be installed by default after pipenv has used
relevant Pipfiles.
We want to ignore the setup.py file if it is around along with a
Pipfile.
We want to use the more specific version, Pipfile.lock, if it is
available. Are we ignoring the Pipfile if it is, as we should?
Apparently testing to import the package will work no matter what as we
will find it locally so setup.py may not need to run. By adding a small
dependency I could check if the dependency is installed and now the
tests actually tests what they should.
Previously we did not check that requirements.txt was ignored properly,
now we do.
Why install a big package when we can install a trivial one?
This is the actual meat while most previous commits were tests to verify
that this worked as intented. The tests paid off quickly as this was
quite hard to get working at all.

- We can not use `pipenv shell` inside this script.
- We must use `--system` and therefore `pipenv install` that supports it
rather than `pipenv sync` that doesn.
- We must generate a `Pipfile.lock` if there are none.
- We must use `--ignore-pipfile` and `--deploy` on the `pipenv install`
command in order to mimic how a `pipenv sync` would have worked.
@jupyter jupyter deleted a comment from meeseeksmachine Apr 21, 2019
@consideRatio consideRatio changed the title [WIP] Support Pipfile / Pipfile.lock with pipenv [Review] Support Pipfile / Pipfile.lock with pipenv Apr 21, 2019
@jupyter jupyter deleted a comment from meeseeksmachine Apr 21, 2019
@jupyter jupyter deleted a comment from meeseeksmachine Apr 21, 2019
@jupyter jupyter deleted a comment from meeseeksmachine Apr 21, 2019
@jupyter jupyter deleted a comment from meeseeksmachine Apr 21, 2019
@jupyter jupyter deleted a comment from meeseeksmachine Apr 21, 2019
@jupyter jupyter deleted a comment from meeseeksmachine Apr 21, 2019
@consideRatio consideRatio changed the title [Review] Support Pipfile / Pipfile.lock with pipenv [WIP] Support Pipfile / Pipfile.lock with pipenv Apr 21, 2019
Give priority to Pipfile.lock, then Pipfile, then runtime.txt.
@consideRatio

This comment has been minimized.

Copy link
Collaborator Author

consideRatio commented Jun 24, 2019

I did a quick iteration on the documentation of Pipfiles - @betatim I think this PR may be close to complete now.

Hmmm okay I realize there may be a changelog etc I should note things in.

consideRatio and others added 9 commits Jun 25, 2019
Fix typo
Co-Authored-By: Tim Head <betatim@gmail.com>
Fix typo
Co-Authored-By: Tim Head <betatim@gmail.com>
The '//' will render to '/' in the Dockerfile that allows the RUN
command to continue on a new line. I initially added a '//' within a
python string on a blank line in order to format things within the
Python file to look potentially nicer, but this is of little importance
and I have now removed it again.
@consideRatio

This comment has been minimized.

Copy link
Collaborator Author

consideRatio commented Jun 25, 2019

Wieeee!

image

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Jun 26, 2019

Merging this now. 👏 for the stamina to see this through!!

@betatim betatim merged commit 6e93cb0 into jupyter:master Jun 26, 2019
5 checks passed
5 checks passed
ci/circleci: build_docs Your tests passed on CircleCI!
Details
ci/dockercloud Your tests passed in Docker Cloud
Details
codecov/patch 96.15% of diff hit (target 20%)
Details
codecov/project 90.4% (+0.22%) compared to 199d14a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@consideRatio

This comment has been minimized.

Copy link
Collaborator Author

consideRatio commented Jun 26, 2019

Wieee thanks for your excellent review @betatim ! ❤️

I merged it to mybinder.org deploy it and it works on mybinder.org now!

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Jun 26, 2019

Noice! Do you want to make a new repository on https://github.com/binder-examples/ that demos this? Then we can send a tweet out as well.

@consideRatio

This comment has been minimized.

Copy link
Collaborator Author

consideRatio commented Jun 26, 2019

@betatim yes good idea! Will do!

@choldgraf

This comment has been minimized.

Copy link
Collaborator

choldgraf commented Jun 26, 2019

yahoooo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.