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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docs: update instructions to install deps with Poetry #9743

Merged
merged 4 commits into from Nov 23, 2022

Conversation

browniebroke
Copy link
Contributor

@browniebroke browniebroke commented Nov 20, 2022

Updating instructions with the latest from the offiical docs.

The previous get-poetry.py and install-poetry.py installers are deprecated.

Fix #9740


馃摎 Documentation previews 馃摎

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

It would be good to research a little more to try to figure it out how to install poetry without required python.install as it was doing before.

get-poetry.py seems to be deprecated right now and builds using it are failing: https://readthedocs.org/projects/test-builds/builds/18691179/

However, using the new installer seems to not install the dependencies in the right virtualenv: https://readthedocs.org/projects/test-builds/builds/18691225/

I'm more tempted to recommend using pipx or pip instead (the other installation methods shown in the official docs), where the instructions will be simpler that the required YAML shown on this PR. What do you think?

@browniebroke
Copy link
Contributor Author

I'm more tempted to recommend using pipx or pip instead (the other installation methods shown in the official docs), where the instructions will be simpler that the required YAML shown on this PR. What do you think?

I totally agree, I'll make the changes now.

One thing I noticed while testing this out is that I had to move poetry install to post_install: I'm using a recent version of Sphinx, and the RTD install was installing sphinx<2 by default, which uninstalled my version (4) because it ran after. I'm a bit surprised that it wasn't an issue before, to be honest.

@humitos
Copy link
Member

humitos commented Nov 21, 2022

I'm using a recent version of Sphinx, and the RTD install was installing sphinx<2 by default, which uninstalled my version (4) because it ran after. I'm a bit surprised that it wasn't an issue before, to be honest.

This is because your project is "old" and we haven't touched those project to avoid breaking them. See https://docs.readthedocs.io/en/stable/build-default-versions.html#python. If you want me to disable this feature flag in your project, please use the from in https://readthedocs.org/support/

@browniebroke
Copy link
Contributor Author

This is because your project is "old" and we haven't touched those project to avoid breaking them.

Yes, I understand: better to remain stable. I can't imagine how many docs pages haven't touched in years and are stuck on the old build system and RTD have to keep this. The great thing is that the config file is flexible enough to let more advanced users customise their build.

I was more sharing my finding to explain why I changed it, compared to how it was documented before.

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

This is close to be merged. I commented about a way to make this example simpler.

I tested this using this YAML file and it worked fine: https://github.com/readthedocs/test-builds/blob/c9923d6ca0d691643be5e9e84806988c156c6135/.readthedocs.yaml#L8-L15, so, I'd expect to use something similar unless there is a reason to not to.

# Install project's dependencies
- $HOME/.poetry/bin/poetry install
- poetry config virtualenvs.create false
post_install:
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 remove this post_install here to simplify the config a little since it's not required for the most common use case. I tried this in test-builds project and it worked properly https://readthedocs.org/projects/test-builds/builds/18704586/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was best to cover more edge cases, in the hope that it would avoid potential issues for old projects and therefore support requests/GitHub issues.

That being said, I don't mind either way & I'll remove this section 馃憤馃徎

docs/user/build-customization.rst Show resolved Hide resolved
docs/user/build-customization.rst Show resolved Hide resolved
docs/user/build-customization.rst Show resolved Hide resolved
docs/user/build-customization.rst Outdated Show resolved Hide resolved
# Install project's dependencies
- $HOME/.poetry/bin/poetry install
- poetry config virtualenvs.create false
post_install:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was best to cover more edge cases, in the hope that it would avoid potential issues for old projects and therefore support requests/GitHub issues.

That being said, I don't mind either way & I'll remove this section 馃憤馃徎

@davidorme
Copy link

Just to cross reference that this recipe closes #9740.

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

This is pretty close to get merged!

@@ -310,7 +310,7 @@ Install dependencies with Poetry
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Projects managed with `Poetry <https://python-poetry.org/>`__,
can use the ``post_create_environment`` user-defined job to use Poetry for installing Python dependencies.
can use the ``post_create_environment`` user-defined job to install Poetry and ``post_install`` to install Python dependencies.
Copy link
Member

Choose a reason for hiding this comment

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

We are not using post_install anymore, so we should revert this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated 馃槃

@humitos
Copy link
Member

humitos commented Nov 23, 2022

@davidorme thanks for confirming this works for your case! 馃挭馃徏

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks a lot 馃挴

@humitos humitos enabled auto-merge (squash) November 23, 2022 10:19
@humitos humitos merged commit 40f253d into readthedocs:main Nov 23, 2022
@browniebroke browniebroke deleted the poetry-builds branch November 23, 2022 15:50
@juantocamidokura
Copy link
Contributor

Hi @browniebroke. Thank you for your contribution. It was really useful for our team. I was planning to open a similar PR regarding this documentation page https://docs.readthedocs.io/en/stable/guides/poetry.html.

Do you think your PR deprecates it? In our investigation on migrating to Poetry it mislead us on how to integrate it with ReadTheDocs.

Thanks in advance 馃槃

@browniebroke
Copy link
Contributor Author

Good question. I think I myself stumbled upon that page a while ago before finding the dedicated example. It might predate the more advance build hooks.

I'm not a maintainer, but my guess is that it would be better to open a new issue for it. Closed issues and pull requests are generally not the best place to have discussions.

@benjaoming
Copy link
Contributor

It would be great if you can open an issue @juantocamidokura !

A lot of our documentation is being updated these days, and it would be great to have some help from Poetry users to update this particular How-to Guide 馃檹

@humitos
Copy link
Member

humitos commented Nov 30, 2022

Hi @browniebroke. Thank you for your contribution. It was really useful for our team. I was planning to open a similar PR regarding this documentation page https://docs.readthedocs.io/en/stable/guides/poetry.html.

This guide was written when build.jobs and build.commands did not exist yet. So, at that time, the only way of using poetry is the one described in that guide. I'm not a poetry user, so I'm not sure which approach is better currently.

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.

Docs build.job recipe for poetry does not work with newer versions
5 participants