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

Support Pipfile #3181

Open
gsemet opened this issue Oct 23, 2017 · 40 comments

Comments

@gsemet
Copy link

commented Oct 23, 2017

I prefer using Pipfile over requirements.txt. Please support this file format to set up the virtual environment.

@RichardLitt

This comment has been minimized.

Copy link
Member

commented Oct 24, 2017

Thanks @stibbons. Contributions are always welcome, of course, if you'd like to submit a PR for this.

@agjohnson

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2017

I started playing with support for pipenv, it looks like it might be a drop in replacement for calls to pip install. We'll probably want to offer the option through our readthedocs.yml file, so this will take:

  • Conditional call to pipenv in doc_builder/python_environments.py
  • readthedocs.yml support in our readthedocs-build repo
  • readthedocs.yml support in this repo
@gsemet

This comment has been minimized.

Copy link
Author

commented Oct 24, 2017

yes, it is a straigh replacement of pip (actually, a wrapper), you do pipenv install thispackage and it installs the package in the virtualenv (using pew) and modify Pipfile accordingly. For ReadTheDocs, I suggest you do pipenv install --dev and ensure the lib is installed in the environment. I usually use -e . to inject my lib under developement into the virtualenv, but I am not sure if everybody does it

@tuukkamustonen

This comment has been minimized.

Copy link

commented Oct 24, 2017

You should actually do pipenv install --dev --ignore-pipfile. Without --ignore-pipfile, pipenv will just seek to upgrade the packages in Pipfile and not really do deterministic build. See pypa/pipenv#954 (comment).

@gsemet

This comment has been minimized.

Copy link
Author

commented Oct 24, 2017

indeed

@agjohnson

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2017

I'm already using pipenv for other projects, so have some familiarity. I believe for our case, we actually want --system as well, to utilize our existing virtualenvs. We have special storage for this, so that venvs persist across builds.

@tuukkamustonen this confuses me a little, but I've only recently started with pipenv and pipfile. What does ignoring the pipfile mean for projects that don't check in their pipfile.lock? It doesn't seem checking in pipfile.lock is a requirement.

I suppose the most correct is check for pipfile.lock, if so, --ignore-pipfile, otherwise don't?

Some of these options will likely be added/allowed override through our readthedocs.yml, --dev is likely one of those.

In the case of a requirements.txt, we should be able to just use pipenv install instead of pip install

@gsemet

This comment has been minimized.

Copy link
Author

commented Oct 24, 2017

Both side is ok. When you freeze the requirements.txt (ex using pip-tools), it act like when you use the lock file.
When you only describe the version by range or without version, it act like Pipfile.
The lock file is specific to the machine, it does not have the markers (ex « python_version < 3 »), but both should work.
I would use ‘pipenv install —dev —system’, it is like ´pip install -r requirements.txt -r requirements-dev.txt’

@tuukkamustonen

This comment has been minimized.

Copy link

commented Oct 25, 2017

@agjohnson I am new to pipenv as well, but as I understand it, if you run just pipenv install it will NOT follow your Pipfile.lock, but instead look into Pipfile, and upgrade any packages that you haven't locked into exact version, and then update Pipfile.lock. So it's like pip-compile and pip-sync (from pip-tools package) combined. I think it's a bit confusing.

I suppose the most correct is check for pipfile.lock, if so, --ignore-pipfile, otherwise don't?

Yeah, if we are after deterministic builds, then the lock-file needs to be followed and --ignore-pipfile given. I don't know how it would make sense otherwise. And then there is pipenv update, that uninstalls/installs packages like pip-sync but it doesn't have --ignore-pipfile option... hmm.

The lock file is specific to the machine, it does not have the markers (ex « python_version < 3 »), but both should work.

This is a good reminder. Neither pip-tools or pipenv (which uses pip-tools underneath) support "universal" lock files. So if you create the lockfile under python 3.6 on Windows and then run it on 3.4 on Linux, things may break, because lockfile expects 3.6/Windows environment. See pypa/pipenv#857 and jazzband/pip-tools#563.

What does ignoring the pipfile mean for projects that don't check in their pipfile.lock

Those projects won't get deterministic builds. It's ok, but should be documented. One of the main benefits with pipenv is to get deterministic builds (improvement over simple requirements.txt), but as you say, it's not a requirement. User should just acknowledge that.

In the case of a requirements.txt, we should be able to just use pipenv install instead of pip install

I don't think pipenv reads requirements.txt-style files. In case there is no Pipfile why not just resort to old behavior?

@agjohnson

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2017

This is a good reminder. Neither pip-tools or pipenv (which uses pip-tools underneath) support "universal" lock files. So if you create the lockfile under python 3.6 on Windows and then run it on 3.4 on Linux, things may break, because lockfile expects 3.6/Windows environment. See pypa/pipenv#857 and jazzband/pip-tools#563.

Ah yes, so it seems that ignoring the Pipfile and relying on Pipfile.lock should be an option, but is off by default. Without universal lock files, we can't guarantee that the environment is the same OS or even python version as our build images.

I don't think pipenv reads requirements.txt-style files. In case there is no Pipfile why not just resort to old behavior?

It looks like if a Pipfile is missing and a requirements.txt is available, pipenv install will automatically translate this to a Pipfile. Likewise, if the file is named foo.txt, pipenv install -r foo.txt will pick up the arbitrarily named file. These are the two modes we need to support on RTD.

@gsemet

This comment has been minimized.

Copy link
Author

commented Oct 25, 2017

It does not change from requirements.txt, it may or may not be reproductible, depending on the work of the programmer... I would not bother, simply use the --dev, because most of the time ReadTheDoc is triggered at the same time than travis.

Later when universal lock file will be settled this can be adapted to Rtd, but this is not as "important" than in the travis build (because the artefact is the doc, not the actual wheel), and we dont have this ability in the travis.

Or, simpler, you let the developer write its install line (pip install -r requirements.txt, or pipenv install --dev), it is the more versatile solution.

@sobolevn

This comment has been minimized.

Copy link

commented Jan 16, 2018

What's the status of this issue? Maybe there is something I can help with?
Thanks!

@DmytroLitvinov

This comment has been minimized.

Copy link

commented May 4, 2018

Hey guys.
Are there any news?

@agjohnson

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2018

No updates. Feel free to take this work on if you want.

@agjohnson agjohnson added this to the Admin UX milestone Jun 1, 2018

@Tobotimus

This comment has been minimized.

Copy link

commented Jun 16, 2018

Hey guys, firstly, thanks for all the work you guys do on this project 😃

I've taken an interest in this issue, and I'd just like to note a few things down about how we expect people will use this feature, and which use cases we should accommodate for. People often get confused about how pipenv should be used, and thus I expect it will be difficult to please everyone with this feature.

I am going to use the information provided in this section of pipenv's documentation as the main point of reference. Accommodating for the use cases and guidelines listed there should satisfy most developers.

RTD's installation process

So currently, there are three stages to the installation process after the virtualenv is created:

  1. Installing RTD's core requirements
  2. Installing user requirements from a requirements.txt file with pip (optional stage)
  3. Installing the package itself with pip install . or python setup.py install --force (also optional)

How pipenv fits into these stages

Installation of pipenv itself should be done in the first stage. I think this should be conditional based on the YAML config.

Now, some (or perhaps even "most") usages of pipenv would actually combine stages two and three, as many projects have -e . or some equivalent specification in their Pipfile (and subsequently their Pipfile.lock, which would also contain sub-dependencies of the package).

However, as specified in the documentation linked above, Pipfiles are also capable of managing dependencies by themselves without relying on the project's install_requires. In this case, pipenv would be used to complete stage 2, and pip install . or python setup.py install for stage 3, if required.

The simplest and most malleable approach I can think of is doing pipenv install {opts} as an additional stage between 2 and 3. Since stages 2 and 3 are already optional, this gives the user lots of freedom for how they'd like to set up the environment, without confusing the user with mutually exclusive configuration options. The documentation would obviously need to be quite clear on how these various configuration options can be used together.

Arguments to pipenv install

I think in the interest of giving the user freedom and also future-proofing the configuration for any new pipenv install flags, we should simply allow the user to specify the flags they'd like to use as a string in the YAML, however we should have --system set as a persistent flag to ensure pipenv doesn't create another virtualenv. Using pipenv to manage virtual environments is unnecessary as we already have a lovely robust design and API for setting up virtual environments with virtualenv, and pipenv's virtual environment management is more of an end-user QoL feature.

I think --ignore-pipfile is unnecessary as the default behaviour is to install from Pipfile.lock.

However, in any case, I think we ought not to specify packages or requirements files as arguments to pipenv install, since that is used for adding dependencies to a Pipfile and not deterministically recreating a development environment.

Config layout

So given the above points, I think my approach would be to add these options to the YAML:

pipenv_install: True|False
pipenv_install_opts: OPTIONS

Edit: On second though perhaps having these options as sub-keys below a pipenv key would be preferable.

The reason I'm not including it in the python key is because I think it should be on the same level as the requirements_file key, which this is most similar to.

Summary of installation with pipenv

  1. Install RTD's core requirements with pip, including pipenv if pipenv_install is True
  2. Optionally install requirements from a requirements.txt file with pip
  3. Optionally do pipenv install --system {opts}
  4. Optionally do either pip install .[extras] or python setup.py install --force, or neither

Please let me know if you have a different view and/or point out improvements which could be made 😃

@stsewd

This comment has been minimized.

Copy link
Member

commented Jun 18, 2018

@Tobotimus thanks, that's a great summary of the use case of pipenv, currently, we are doing a v2 of the configuration file https://github.com/orgs/rtfd/projects/2, I think this should be there.

@Tobotimus

This comment has been minimized.

Copy link

commented Jun 19, 2018

@stsewd Ah I see! Great. Unfortunately that link is a 404 for me, and I can't see any projects on rtfd's org page, however I did find the YAML file completion milestone, which I'm happy to help out with when I get some time 😄 I am a new here so still somewhat stumbling around.

I've opened up #4254 as a patch to the old config, but I am more than happy to refactor it according to any new spec. Is there a feature branch of some sort for the new config? Thanks!

@stsewd

This comment has been minimized.

Copy link
Member

commented Jun 19, 2018

We are moving the rtd-build repo to the readthedocs.org repo #4242, there are some ideas here https://docs.readthedocs.io/en/latest/design/yaml-file.html and this is kind of the base schema we have for the v2 https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/rtd_tests/fixtures/spec/v2/schema.yml. There are some design decisions to make.

@Tobotimus

This comment has been minimized.

Copy link

commented Jun 19, 2018

Thanks very much, I suppose my approach from #4254 could easily adapt to that schema by simply placing the pipenv section under the python key. For reference, here is the patch I prepared for the rtd-build repo. I suppose I will wait for #4242 to be merged and for more progress to be made on the V2 config parser before continuing with pipenv PRs 😃

@agjohnson

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2018

There is more work to finish up the porting process, so it would probably be best to wait for that process to be completed first. I suppose we can consider readthedocs_build frozen for now.

Once things are ported, you can rework your PR to be housed here.

For now, a separate PR that updates our v2 schema spec file would be the best way to start. You'll find tests and the schema here:
https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/rtd_tests/fixtures/spec/v2/schema.yml
https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/rtd_tests/tests/test_build_config.py

This would be the best place to describe your changes and rationale for schema changes.

The immediate feedback I'd have is that I don't like exposing a pipenv.options, we don't do this for any other tools, we rather support specific options. I agree that this should all be under a python.pipenv key, but perhaps we also need to change python.install to support pipen instead of pip.

Edit: cc @Tobotimus, also 🎉 for the contribution! Sorry your PR landing right in the middle of a large project

@agjohnson agjohnson added Feature and removed Improvement labels Jun 19, 2018

@stsewd

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

I'm testing pipenv now, I found this:

  • --ignore-pipfile will try to generate a lock file if one isn't found.
  • When running pipenv install, it will generate a lock file.
  • When running with --skip-lock it will install from pipfile, without generating a lock

If an error happens when generating the lock file, it doesn't install the dependencies (it may fail if there are incompatible dependencies).

So, people using un-pined dep (like libraries), may have this problem, should it be a problem to be fixed by the users? Or should we put both options? --ignore-pipfile and --skip-lock? (--ignore-pipfile will be on by default)

@stsewd

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

Pipenv doesn't support specify a custom Pipfile (we plan to support installations from other than roo directory). So, I'm planning to put that as an environment variable, the command looks like this

 PIPENV_PIPFILE=./custom/path/Pipfile pipenv install --system --dev --ignore-pipfile --skip-lock 

--skip-lock and --ignore-pipfile are options on the yaml file, --ignore-pipfile is on by default. Also, --dev is an option and is off by default.

With this we install the dependencies in the same virtual env. I'm using a configuration like this for this project https://github.com/gsemet/cfgtree:

version: 2
sphinx:
  configuration: docs/conf.py
python:
  install:
    - pipfile: .
      skip_lock: true
      dev: true
    - path: .
      method: pip
@stsewd

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

Hey folks, I'm testing the pipfile support on rtd, can you please comment with a project that is using a pipfile (and have docs to build)? That will help me a lot on testing and bring a less buggy implementation p:

@willrogers

This comment has been minimized.

Copy link

commented Oct 29, 2018

https://github.com/dls-controls/pytac uses Pipenv, has a pipfile, but also has a rtd-requirements.txt file for this exact purpose. It's at https://pytac.readthedocs.io/en/latest/.

Thanks for looking at this!

@gsemet

This comment has been minimized.

Copy link
Author

commented Oct 29, 2018

That’s the role of pipenv-to-requirements/ :)

@stsewd

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

@willrogers I was able to build your docs with the current implementation, thanks!

@stale

This comment has been minimized.

Copy link

commented Jan 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale label Jan 10, 2019

@Peque

This comment has been minimized.

Copy link

commented Jan 10, 2019

@stsewd Any updates on this matter? 😊

@stale stale bot removed the Status: stale label Jan 10, 2019

@stsewd stsewd added Accepted and removed Needed: patch labels Jan 10, 2019

@stsewd

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

Sorry for the bot, this isn't stale. We are blocked only by #4740, which only needs another review to go. Then we can merge #4783 in top of that. We are pretty close to get this feature out.

@stsewd stsewd modified the milestones: 3.2, 3.4 Mar 6, 2019

@stsewd stsewd modified the milestones: 3.4, 3.5 Mar 27, 2019

@RohanNagar

This comment has been minimized.

Copy link

commented May 1, 2019

Hi all, is there any update on this? Looks like #4740 was merged, so can #4783 be merged in as well? Really looking forward to this feature.

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