feat: use uv lock file - #1427
Conversation
17f742f to
6932c1a
Compare
| python-version: ${{ env.MAIN_PYTHON_VERSION }} | ||
| use-python-cache: false | ||
| checkout: false | ||
| skip-install: false |
There was a problem hiding this comment.
Adding a comment to remind the need to revert some CI changes before any merge
moe-ad
left a comment
There was a problem hiding this comment.
Thanks for the changes @SMoraisAnsys, I left a couple of comments.
There is one other thing I have been thinking about, why can't we just use the uv lock files automatically once detected rather than exposing an explicit input for it (similar to how there is no input for poetry). IMO, a project that already uses a lock file would expect its dependencies to be install from it.
| operating-system: ${{ matrix.os }} | ||
| python-version: ${{ env.MAIN_PYTHON_VERSION }} | ||
| attest-provenance: true | ||
| use-uv-lockfile: ${{ matrix.repository.name == 'pyansys-geometry' }} |
There was a problem hiding this comment.
I suggest creating a separate workflow (ci_cd_pr_uv ?) for testing projects that have started moving to uv to keep things clean and separated, and to have a natural place for testing more uv projects in the future (quite a number of them now). What do you think?
Although pyansys-geometry is using a hatchling build backend instead of uv_build, we could still test all projects leveraging uv lock files but not using any of flit_core.buildapi/poetry.core.masonry.api build backends in ci_cd_pr_uv.
There was a problem hiding this comment.
That was just a CI config to illustrate that the changes work. Having another test workflow is a good idea but shouldn't we wait for uv_build to be used ?
| env: | ||
| INSTALL_COMMAND: ${{ inputs.use-uv == 'true' && 'uv pip install --no-managed-python' || 'python -m pip install' }} | ||
| INSTALL_TARGET: ${{ steps.specific-target-requested.outputs.install_target }} | ||
| TARGET: ${{ inputs.target }} |
There was a problem hiding this comment.
INSTALL_TARGET can be used directly rather than creating a separate env variable.
There was a problem hiding this comment.
Unfortunately this is not possible because INSTALL_TARGET would be . or .[my-target] (I just tested the changes :') ) Also, this env var is used when creating the wheel so better keep things as they are.
| source_code_folder = None | ||
| # Read the .pth file to find the source code path | ||
| with Path(pth_file).open("r") as file: | ||
| with Path(str(dist.locate_file(pth_file))).open("r") as file: |
There was a problem hiding this comment.
Out of curiosity, why this change? Did the previous implementation start failing?
There was a problem hiding this comment.
This is described in the PR description. Let me know if that's not clear enoguh
| python-version: ${{ env.MAIN_PYTHON_VERSION }} | ||
| attest-provenance: true | ||
| use-uv-lockfile: ${{ matrix.repository.name == 'pyansys-geometry' }} | ||
| use-uv: ${{ matrix.repository.name == 'pyansys-geometry' }} |
There was a problem hiding this comment.
| use-uv: ${{ matrix.repository.name == 'pyansys-geometry' }} |
use-uv is true by default in the actions, which means we were always testing this execution path for every project. This change now means it will be only be tested for pyansys-geometry, which is not what you intended I suppose.
There was a problem hiding this comment.
This is a temporary ci config and planned to be reverted (cf #1427 (comment))
I'm fine with detecting it and activating it by default. My only concern is the sudden change of behavior for people that still work with version tag and/or don't read every release note carefully. |
Follow up of #1364 but with a clean branch.
Related to #1088, in particular this comment, and #1088.
Testing in pyaedt was successfull and highlighted that
fonttools, a transitive dependency, was associated to a vulnerability in Safety's db. This is not a universal id like CVE/GHSA.The use of
uv add --group ansys-actions -r "${REQUIREMENTS_FILE}"has been removed in 10ddec7 because some hooks might check the pyproject.toml / uv.lock files and lead to CI failure. That is the case withpyansys-geometrywhich leveragespyproject-fmt.The smoke_test.py file has been updated in 6932c1a to be compatible with
uv's architecture to manage packages. I assume that we we weren't affected previously becauseuv pip installmimicspip's behavior. But when usinguv syncthat behavior changes and we can't useCWDto retrieve the dist files. Levering relative location solves this issue and seems to also work with other package managers.Note
Currently the changes are limited to a subset of actions. We might want to extend it to others. TBD