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

fix: doc build without python cache #543

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

SMoraisAnsys
Copy link
Contributor

@SMoraisAnsys SMoraisAnsys commented Aug 19, 2024

Following a discussion with @RobPasMue, the implementation of the doc-build action did not correctly take into account the fact of not using caching when installing Python packages.

Note: this is particularly annoying when one works with self-hosted runners where you can easily think that you are using the latest version of a package and it's not the case.

This PR does:

  • add --no-cache-dir flag for pip build backend or --no-cache flag for poetry backend if the input asks for no python cache;
  • refactoring related to determining whether the backend is poetry or pip.

Extra: to handle poetry and virtual environment, I use poetry config virtualenvs.create false. This configures poetry to not create a new virtual environment and ensure it uses the activated virtual environment. Another approach would have been to use poetry's capability to create a virtual environment in-project. However, this wouldn't work well in the case of our VM with self hosted runners. Indeed, if a virtual environment has already been created for the project under {cache-dir}/virtualenvs, using poetry config virtualenvs.in-project true will not cause poetry to create or use a local virtual environment.
To fix that, a manual deletion of the virtual environment is required but it would troublesome in the long run... Thus, this option is not leveraged.

@SMoraisAnsys SMoraisAnsys requested a review from a team as a code owner August 19, 2024 13:59
@ansys-reviewer-bot
Copy link
Contributor

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review

@SMoraisAnsys SMoraisAnsys marked this pull request as draft August 19, 2024 13:59
@github-actions github-actions bot added the bug Defects or glitches reported by users or developers label Aug 19, 2024
Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

Some comments :) otherwise LGTM

_doc-build-linux/action.yml Outdated Show resolved Hide resolved
Comment on lines 228 to 231
source .venv/bin/activate
if [[ ${{ env.BUILD_BACKEND }} == 'poetry' ]]; then
python -m pip install poetry ${{ env.PYTHON_NO_CACHE_OPTION }}
poetry install --with doc ${{ env.PYTHON_NO_CACHE_OPTION }}
Copy link
Member

Choose a reason for hiding this comment

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

venvs and poetry don't get along well AFAIK... Have you tried this with a poetry based project? Also... where is the venv created? I can't see it in this file...

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'll need to check on such repo yeah, I didn't tried yet :)

doc-build/action.yml Show resolved Hide resolved
# ------------------------------------------------------------------------

- name: Documentation build (Linux)
if: ${{ runner.os == 'Linux' }}
uses: ansys/actions/_doc-build-linux@main
uses: ansys/actions/_doc-build-linux@fix/doc-build-without-python-cache
Copy link
Member

Choose a reason for hiding this comment

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

Please remember to change this

@@ -212,7 +218,7 @@ runs:

- name: Documentation build (Windows)
if: ${{ runner.os == 'Windows' }}
uses: ansys/actions/_doc-build-windows@main
uses: ansys/actions/_doc-build-windows@fix/doc-build-without-python-cache
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@SMoraisAnsys
Copy link
Contributor Author

FYI, I tested this on a fork of pyconceptev as I think poetry handling might have been difficult.

The PR : SMoraisAnsys/pyconceptev#1

The CI associated consists in running multiple workflows where I build the documentation twice:

  • Windows/Linux : build documentation with use-python-cache = True, remove virtual environment and build documentation with use-python-cache = True. The packages are downloaded for the first documentation build and not downloaded for the second documentation build.
  • Windows/Linux : build documentation with use-python-cache = True, remove virtual environment and build documentation with use-python-cache = False. The packages are downloaded every time to build the documentation!

The idea of those workflow is to mock how a VM would behave when it had been already used with caching.

The logs are here, know that I manually checked and it was consistent.

@SMoraisAnsys
Copy link
Contributor Author

Tested on pyansys-sound and it worked, see ansys/pyansys-sound#111

@SMoraisAnsys
Copy link
Contributor Author

Tested in pyansys-geometry and working as expected, see ansys/pyansys-geometry#1404

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Defects or glitches reported by users or developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants