-
Notifications
You must be signed in to change notification settings - Fork 6
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
Conversation
Thanks for opening a Pull Request. If you want to perform a review write a comment saying: @ansys-reviewer-bot review |
There was a problem hiding this 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
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 }} |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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 :)
# ------------------------------------------------------------------------ | ||
|
||
- 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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
Co-authored-by: Roberto Pastor Muela <[email protected]>
FYI, I tested this on a fork of The PR : SMoraisAnsys/pyconceptev#1 The CI associated consists in running multiple workflows where I build the documentation twice:
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. |
Tested on |
Tested in |
Closing following #621 |
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:
--no-cache-dir
flag forpip
build backend or--no-cache
flag forpoetry
backend if the input asks for no python cache;poetry
orpip
.Extra: to handle
poetry
and virtual environment, I usepoetry 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 usepoetry
'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
, usingpoetry 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.