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

Add support for arbitrary package names in python.install #5572

Open
webknjaz opened this issue Apr 4, 2019 · 53 comments
Open

Add support for arbitrary package names in python.install #5572

webknjaz opened this issue Apr 4, 2019 · 53 comments
Labels
Accepted Accepted issue on our roadmap Feature New feature Needed: design decision A core team decision is required

Comments

@webknjaz
Copy link
Contributor

webknjaz commented Apr 4, 2019

I was trying to follow https://docs.readthedocs.io/en/stable/config-file/v2.html#packages but it seems to miss one important use-case.

My project requires pip 19 for building (because of PEP517 dep).
So I tried to do this:

version: 2
...
python:
  ...
  install:
  - method: pip
    path: pip >= 19.0.3
  ...

And it failed because the validator expects a file system path.

I however need this and I don't want to create a requirements file just for this. It also doesn't make sense to put it into dist extras because it needs to be there before building the dist.

I think it should be pretty easy to extend the config. OTOH just having an up-to-date Pip version capable of PEP 517 would also deal with my problem.

Details

Expected Result

Build env have pip>=19.0.3

Actual Result

Failure to parse config

webknjaz added a commit to sanitizers/octomachinery that referenced this issue Apr 4, 2019
@stsewd
Copy link
Member

stsewd commented Apr 4, 2019

We already install the latest version of pip, this is done just after the virtual env creation and before the package/requirements installation.

@webknjaz
Copy link
Contributor Author

webknjaz commented Apr 4, 2019

Yeah, I see that now. Now I realized that I'm hitting Pip's bug: basically, setuptools from system site-packages leak into the isolated build...

@webknjaz
Copy link
Contributor Author

webknjaz commented Apr 4, 2019

As you can see, bumping things in the virtualenv manually doesn't help: https://readthedocs.org/projects/octomachinery/builds/8869252/

@webknjaz
Copy link
Contributor Author

webknjaz commented Apr 4, 2019

Looks like pypa/pip#6264

@webknjaz
Copy link
Contributor Author

webknjaz commented Apr 4, 2019

By the way, did anyone suggest providing the ability to just specify tox env for building things? This would save me because I've made it work there.

@webknjaz
Copy link
Contributor Author

webknjaz commented Apr 4, 2019

Anyway, I think bumping setuptools/pip in the system site-packages would "fix" this. At least, it seems like a valid workaround.

@hroncok
Copy link

hroncok commented Apr 4, 2019

Bumping system site-packages setuptools to at least 40.8.0 or removing it entirely works as a workaround.

@webknjaz
Copy link
Contributor Author

webknjaz commented Apr 4, 2019

Nice, but in my case, it's 40.9.0 which is required because I rely on a feature introduced there (missing setup.py, having only setup.cfg).

@stsewd
Copy link
Member

stsewd commented Apr 4, 2019

The pip in the global system package comes from the default in ubuntu https://github.com/rtfd/readthedocs-docker-images/blob/71714aa45ffd2b00dc599fd1353651a706ed9f49/Dockerfile#L124-L125

@stsewd
Copy link
Member

stsewd commented Apr 4, 2019

By the way, did anyone suggest providing the ability to just specify tox env for building things? This would save me because I've made it work there.

I don't think we will support that, we depend on a specific workflow, you may find this interesting #1083

@stsewd stsewd added the Support Support question label Apr 4, 2019
@webknjaz
Copy link
Contributor Author

webknjaz commented Apr 4, 2019

Yeah... I see. I know I could migrate to gh-pages but if it's possible to fix this on RTD, maybe we should try.
I'm curious whether you could inject pip install -U pip setuptools --user so that it'd use user install. Maybe It'd help. I also see you use pyenv so probably "system" site-packages are coming from there, not OS, because OS-installed things probably point to a different version of Python anyway. So perhaps you should update not what installed with apt, but Python which is userspace-installed by pyenv...

@stsewd
Copy link
Member

stsewd commented Apr 5, 2019

If the buggy lib is coming from the installation in pyenv, it would be updated the next time we update the docker images

https://github.com/rtfd/readthedocs-docker-images/blob/71714aa45ffd2b00dc599fd1353651a706ed9f49/Dockerfile#L177-L178

@webknjaz
Copy link
Contributor Author

webknjaz commented Apr 5, 2019

@stsewd you update pip there but not setuptools

@webknjaz
Copy link
Contributor Author

webknjaz commented Apr 5, 2019

@stsewd it would be nice to indicate which docker image was used on the build page...
Can you please tell me how can I reproduce this locally? Which image should I use? latest one year outdated (#5553), stable doesn't have 3.7. I'm guessing testing?

@webknjaz
Copy link
Contributor Author

webknjaz commented Apr 5, 2019

Alright, it looks like latest in config points to 4.x branch...

@webknjaz
Copy link
Contributor Author

webknjaz commented Apr 5, 2019

I've also found another issue in the build log:

/home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/bin/python -m pip install --upgrade --cache-dir /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/.cache/pip Pygments==2.3.1 setuptools<41 docutils==0.14 mock==1.0.1 pillow==5.4.1 alabaster>=0.7,<0.8,!=0.7.5 commonmark==0.8.1 recommonmark==0.5.0 sphinx<2 sphinx-rtd-theme<0.5 readthedocs-sphinx-ext<0.6

Requirement already up-to-date: Pygments==2.3.1 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (2.3.1)
Requirement already up-to-date: setuptools<41 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (40.9.0)
Requirement already up-to-date: docutils==0.14 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (0.14)
Requirement already up-to-date: mock==1.0.1 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (1.0.1)
Requirement already up-to-date: pillow==5.4.1 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (5.4.1)
Requirement already up-to-date: alabaster!=0.7.5,<0.8,>=0.7 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (0.7.12)
Requirement already up-to-date: commonmark==0.8.1 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (0.8.1)
Requirement already up-to-date: recommonmark==0.5.0 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (0.5.0)
Requirement already up-to-date: sphinx<2 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (1.8.5)
Requirement already up-to-date: sphinx-rtd-theme<0.5 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (0.4.3)
Requirement already up-to-date: readthedocs-sphinx-ext<0.6 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (0.5.17)
Requirement already satisfied, skipping upgrade: future in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (from commonmark==0.8.1) (0.17.1)
Requirement already satisfied, skipping upgrade: babel!=2.0,>=1.3 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (from sphinx<2) (2.6.0)
Requirement already satisfied, skipping upgrade: imagesize in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (from sphinx<2) (1.1.0)
Requirement already satisfied, skipping upgrade: packaging in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (from sphinx<2) (19.0)
Requirement already satisfied, skipping upgrade: six>=1.5 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (from sphinx<2) (1.12.0)
Requirement already satisfied, skipping upgrade: snowballstemmer>=1.1 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (from sphinx<2) (1.2.1)
Requirement already satisfied, skipping upgrade: requests>=2.0.0 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (from sphinx<2) (2.21.0)
Requirement already satisfied, skipping upgrade: sphinxcontrib-websupport in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (from sphinx<2) (1.1.0)
Requirement already satisfied, skipping upgrade: Jinja2>=2.3 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (from sphinx<2) (2.10)
Requirement already satisfied, skipping upgrade: pytz>=0a in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (from babel!=2.0,>=1.3->sphinx<2) (2018.9)
Requirement already satisfied, skipping upgrade: pyparsing>=2.0.2 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (from packaging->sphinx<2) (2.3.1)
Requirement already satisfied, skipping upgrade: urllib3<1.25,>=1.21.1 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (from requests>=2.0.0->sphinx<2) (1.24.1)
Requirement already satisfied, skipping upgrade: chardet<3.1.0,>=3.0.2 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (from requests>=2.0.0->sphinx<2) (3.0.4)
Requirement already satisfied, skipping upgrade: idna<2.9,>=2.5 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (from requests>=2.0.0->sphinx<2) (2.8)
Requirement already satisfied, skipping upgrade: certifi>=2017.4.17 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (from requests>=2.0.0->sphinx<2) (2019.3.9)
Requirement already satisfied, skipping upgrade: MarkupSafe>=0.23 in /home/docs/checkouts/readthedocs.org/user_builds/octomachinery/envs/latest/lib/python3.7/site-packages (from Jinja2>=2.3->sphinx<2) (1.1.1)

for some reason, it installs setuptools<41

@webknjaz
Copy link
Contributor Author

webknjaz commented Apr 5, 2019

Ah, that's fine...

@webknjaz
Copy link
Contributor Author

webknjaz commented Apr 5, 2019

So I've tried out testing tag and doing pip3.7 install -U setuptools against the pyenv-managed Python outside the virtualenv helped.

@stsewd when should we expect

P.S. btw a bit unrelated: since you're already using pyenv why don't you use its pyenv-virtualenv plugin for managing venvs?

webknjaz added a commit to sanitizers/octomachinery that referenced this issue Apr 5, 2019
@stsewd
Copy link
Member

stsewd commented Apr 5, 2019

I'm not really sure about the docker images in dockerhub, I'm using them without any problem, I guess they work for me bc I added them in a custom dockerfile.

I time ago I tried updating pip from the global system/pyenv, but I was facing some complications (I don't remember exactly), that's why I ended up updating pip inside the virtualenv. I'm going to try to play around updating setuptools and see if it works.

P.S. btw a bit unrelated: since you're already using pyenv why don't you use its pyenv-virtualenv plugin for managing venvs?

I guess for historical reasons, when we weren't using it? Also, we don't really activate the virtualenv, we just use the executable from there. I was thinking more in using the venv module for python3, but we still support python2 builds, so isn't an easy refactor.

@webknjaz
Copy link
Contributor Author

webknjaz commented Apr 5, 2019

@stsewd
Copy link
Member

stsewd commented Apr 5, 2019

What version of sphinx are you using locally?

@webknjaz
Copy link
Contributor Author

webknjaz commented Apr 5, 2019

latest

@webknjaz
Copy link
Contributor Author

webknjaz commented Apr 5, 2019

I think I've figured it out. I'll need to exec and restart it. Because old sphinx is already in memory which messes things up...

@stsewd
Copy link
Member

stsewd commented Apr 5, 2019

We install 1.8.x by default, we don't support 2.0 yet, so you may experiment some weird things (maybe)

@webknjaz
Copy link
Contributor Author

webknjaz commented Apr 5, 2019

I improved the hack: https://github.com/sanitizers/octomachinery/blob/819713b/docs/conf.py#L14-L65

And it worked: https://readthedocs.org/projects/octomachinery/builds/8872645/

don't repeat this at home!

@webknjaz
Copy link
Contributor Author

webknjaz commented Apr 5, 2019

Yeah, it says Sphinx 2.0.0 https://docs.octomachinery.dev/ here in the bottom. I didn't set any version limitations. But the problem with hack was replacing things which have already been loaded into runtime via a separate process...

@stsewd stsewd added Feature New feature Needed: design decision A core team decision is required labels Apr 25, 2019
@stsewd stsewd reopened this Apr 25, 2019
@agjohnson agjohnson changed the title Allow specifying dist names in .readthedocs.yml Add support for arbitrary package names in python.install Jul 23, 2019
@agjohnson agjohnson added Accepted Accepted issue on our roadmap and removed Needed: design decision A core team decision is required labels Jul 23, 2019
@agjohnson
Copy link
Contributor

This was a feature that we cut from the first implementation of our config file, but i think it's important for users to have this functionality.

We rely too heavily on packaging patterns of yore, like pushing users towards requirements files, and this doesn't translate to a first class experience for users using more elegant solutions that exist for packaging now. I'd also advocate for enabling editable installs, so that pip install --editable '.[docs]' is possible without installing the package (install only extras), though this is a separate issue and can be at least remedied by allowing arbitrary package installation. So, I'd vote that this is the next feature we enable in our python.install config.

Removing design decision we discussed the need for this and we just need to assign priority now.

@humitos humitos added the Sprintable Small enough to sprint on label Feb 6, 2020
@ericholscher
Copy link
Member

I just ran into this issue, it would indeed be useful 👍

@agjohnson
Copy link
Contributor

Same, I hit this yesterday and found it redundant that I had to make a separate requirements file just for a single package. I don't think we have anything blocking us from implementing this now that we already support a list of requirements files.

@stsewd stsewd added the Needed: design decision A core team decision is required label Jul 6, 2020
@stsewd
Copy link
Member

stsewd commented Jul 6, 2020

We still need a design decision on how the scheme will look like. One suggestion I have is

python:
  install:
      - packages:
            - mkdocs>=1.1
            - sphinx

@webknjaz
Copy link
Contributor Author

webknjaz commented Jul 6, 2020

Sounds good. But don't do too much validation as pip supports many formats including dist @ http://...

@mcarans
Copy link

mcarans commented Feb 22, 2022

@stsewd The format you suggested looks good to me

@agjohnson
Copy link
Contributor

I'd like to push this issue forward, it's been in design decision for a while.

We still need a design decision on how the scheme will look like.

Looks great, that feel very natural with the rest of our python.install options.

Are there any design decisions left on this, or are we ready to toss this on the roadmap?

@stsewd
Copy link
Member

stsewd commented Mar 30, 2022

Are there any design decisions left on this, or are we ready to toss this on the roadmap?

With the new build.jobs feature, this could also be done with

version: 2

build:
     os: ubuntu-22.04
     tools:
       python: "3.10"
    jobs:
      post_install:
       - pip install nox

@agjohnson
Copy link
Contributor

Yeah this is another option users will have soon, and mostly functionally the same. I might still be +0.5 on implementation of python.install[].packages, as it feels like a little bit of a gap in our configuration file though. Like, "use python.install to install python packages, unless you need a single package, then use build.jobs.post_install" is a bit confusing.

I don't feel too strongly about this though.

@humitos
Copy link
Member

humitos commented Mar 31, 2022

I don't feel strongly either at this point. I see python.install.packages cleaner for beginners and build.jobs.post_install easier for advanced users that already know pip's usage. However, I don't see a huge difference between both of them. Basically, it's a pip install prefix that you will need to add to the packages you want if you are a beginner.

I wouldn't rush implementing this yet and I'd see how build.jobs behaves in the near future, check the feedback we receive and see what happens when we suggest a beginner user to use buil.jobs.post_install to install a single package.

Adding a new config key has a lot of overhead: implementation, maintenance, documentation, and narrative around it as well. If we can get the same output without implementing a new config key, I'm fine testing how it goes with the build.jobs first that we have already implemented it.

@agjohnson
Copy link
Contributor

Yeah, I think this is just about providing first class Python support in the end. We don't need to offer anything but requirements.txt installation, technically. But it's better UX to support more native installation options so users don't have to guess or make hacks.

I agree this isn't a huge priority with build.jobs, its just is a gap we never closed in our implementation.

@thesved
Copy link

thesved commented Dec 16, 2022

Based on the discussion and feedback, what is the current plan for addressing the gap in native Python installation options and providing better UX for beginners?

@humitos humitos removed the Sprintable Small enough to sprint on label Aug 1, 2023
@humitos
Copy link
Member

humitos commented Aug 26, 2023

IMHO, I'd say we should not implement this feature request because it's basically re-implementing a requirements.txt file and we will need to deal with lot of edge cases and complex URLs parsing. There won't be too much difference for a user between python.install.packages and python.install.requirements since the content of python.install.packages would be the same as the content of the requirements.txt file specified in python.install.requirements.

Besides, we will be moving people away from a default and pretty-well established pattern where they can find lot of help/troubleshooting guides on internet.

Finally, we are making our platform more useful for other documentation tools and languages as well, so there will be a feature difference between Python and NodeJs, for example, where we won't support the same for npm install.

I'd close the issue as "not planned".

@agjohnson
Copy link
Contributor

We likely have enough package installation options to get around the need for a one-off package installation option. Good point about Node, is the same true for Rust and one-off package installation?

However, I will also say that I still feel we shouldn't be pushing users towards requirement files where we don't have to either. We have had feedback from Python package maintainers that requirements files are very redundant and something they have to maintain solely for RTD. In addition, modern Python tooling brings lock file support, something you have to hack up when relying on requirements files.

To me, requirements files are the last resort option. For Python packaging projects and likely a good subset of projects with maintainers more familiar with Python, there are much better alternatives to both one-off package installation calls and requirements files. But, pure-documentation projects with less familiarity with Python/Node/etc are likely much more ambivalent to these patterns in all regards.

I support calling this not planned if we add specific workarounds in our documentation using build.jobs. We can call adding direction to our documentation on package management future documentation improvements, as we likely want to consider user/project personas a bit more. I think we do have some room to offer up better solutions to projects

@humitos
Copy link
Member

humitos commented Aug 29, 2023

I support calling this not planned if we add specific workarounds in our documentation using build.jobs.

This is what I'm saying. There is no need to build such a complex feature (which also will work only for Python projects) when we can just document something like:

build:
  os: "ubuntu-22.04"
  jobs:
    pre_build:
      - pip install "sphinx<7.3" sphinx-rtd-theme==1.2.2 sphinx-notfound-page

It uses standards tools (pip) which has a lot more benefits than the wrapper we can build around -- mainly gives users support from the whole community.

@agjohnson
Copy link
Contributor

Sure, and I understood that was the workaround we've been thinking of here. I'm just noting we should update our documentation to satisfy this issue.

This workaround is fine for now, but I'd also still say this isn't great UX though. We'll be pointing users to our python configuration options for some usage of pip and build.jobs for other uses of pip.

We would want to revisit this if we ever add Node or Rust package install options to the config. Node and Rust already won't have parity with our existing Python options like python.install.requirements/etc, so we can't too strictly let our Python install options guide future implementations for other languages. To add any native package installation for these, we probably are looking at single package install calls.

This is all something to consider for the future and to continue monitoring. We can revisit this issue if project configuration is dictating we should support single package installs for any language.

@webknjaz
Copy link
Contributor Author

@humitos I noticed that pre-build is incompatible with reproducible installs as RTD would still inject other pip install commands that are separate and would change some of the installed packages.

So to achieve true reproducibility and complete env provisioning control one is forced to resort to using commands and reproducing the runtime bits as well:
ansible/ansible-documentation@61e2c1#diff-03efc769b870804394632e45d7885272b44c16939517fb31c9d7c614d2ffae57R13-R21.
Of course, the cost for this kind of flexibility is having to sync some of the RTDs commands manually. So it'll look like:

version: 2

build:
  os: ubuntu-22.04
  tools:
    python: >-
      3.11

  commands:
  - python -m venv "${READTHEDOCS_VIRTUALENV_PATH}"
  - >-
    "${READTHEDOCS_VIRTUALENV_PATH}"/bin/python -m pip install --exists-action=w -r tests/requirements.in -c tests/requirements.txt
  - >-
    "${READTHEDOCS_VIRTUALENV_PATH}"/bin/python -m sphinx
    -j auto
    -W
    -n
    --keep-going
    -T
    -E
    -b html
    -d "${READTHEDOCS_OUTPUT}"/_build/doctrees
    -D language=en
    docs/
    "${READTHEDOCS_OUTPUT}"/html

@agjohnson
Copy link
Contributor

@webknjaz Currently, you do need to use build.commands if you need to override all installation commands, including the RTD default commands or commands triggered through configuration options.

In the future, we will surface a build.jobs.install job that can be used to remove all implied commands, but still avoid the need to replicate the entire build process with build.commands.

However, python.install.package wouldn't yet work around this particular issue either, as the implied pip install commands would still execute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Feature New feature Needed: design decision A core team decision is required
Projects
Status: Planned
Development

No branches or pull requests

8 participants