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 scheduled workflow configuration to update pixi lockfile #221

Conversation

flferretti
Copy link
Collaborator

@flferretti flferretti commented Aug 20, 2024

This PR adds a CI configuration to automatically update the pixi.lock with LFS support.

For the time being, since we have a PyPI dependecies, the workflow will take a bit more since it will try to install the update dependencies in the CI environment. We can re-add the --no-install option removed in ec00306 as soon as conda-forge/staged-recipes#27137 gets merged.

Note that it will be necessary to allow Github Actions to create/approve PRs from Settings ➡️ Actions ➡️ General (already checked with current settings):

image

Example PR: flferretti#14

Solves #199


📚 Documentation preview 📚: https://jaxsim--221.org.readthedocs.build//221/

@flferretti flferretti self-assigned this Aug 20, 2024
@flferretti flferretti linked an issue Aug 20, 2024 that may be closed by this pull request
@flferretti flferretti force-pushed the 199-add-a-scheduled-workflow-to-automatically-update-the-pixilock-file branch from 7da31bc to 40a184f Compare August 21, 2024 07:52
@flferretti flferretti force-pushed the 199-add-a-scheduled-workflow-to-automatically-update-the-pixilock-file branch from ec00306 to 40d275c Compare August 21, 2024 10:07
@flferretti flferretti marked this pull request as ready for review August 21, 2024 10:08
Copy link
Member

@diegoferigo diegoferigo left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @flferretti! By chance, did you manage to test this action on your fork as discussed f2f? If not, no worries, we can merge and fix it upstream in a new PR.

.github/workflows/update_pixi_lockfile.yml Show resolved Hide resolved
pyproject.toml Outdated
dependencies = { cuda-version = "12.*", cuda-cupti = "*", jaxlib = "**cuda*" }
dependencies = { cuda-version = "12.*", cuda-cupti = "*" }
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for including this change in this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

jaxlib = "**cuda*" gives a TOML parse error. Moreover, it is not necessary to force the cuda version since the GPU version of jaxlib will be installed automatically if the __cuda metapackage is available.
I tested locally that with that change, the environment produces actually supports GPU:

$ pixi run -e gpu python                            
 WARN The feature 'style' is defined but not used in any environment
 WARN The feature 'testing' is defined but not used in any environment
 WARN The feature 'viz' is defined but not used in any environment
 WARN The feature 'all' is defined but not used in any environment
Python 3.12.4 | packaged by conda-forge | (main, Jun 17 2024, 10:23:07) [GCC 12.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import jax; jax.devices()
[cuda(id=0)]

Copy link
Member

@diegoferigo diegoferigo Aug 21, 2024

Choose a reason for hiding this comment

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

Mmh ok. However, sometimes upstream problems / conflicts may propagate downstream preventing the installation of the cuda version. It'd be safer to keep this specification IMO, otherwise the environment would silently build w/o GPU support. What about removing one of the leading *? Would it be a valid TOML entry?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but it will not find any matching package:

$ pixi run -e gpu python
 WARN The feature 'style' is defined but not used in any envi
ronment
 WARN The feature 'testing' is defined but not used in any en
vironment
 WARN The feature 'viz' is defined but not used in any enviro
nment
 WARN The feature 'all' is defined but not used in any enviro
nment
  × failed to solve the conda requirements of 'gpugroup'
  │ 'linux-64'
  ╰─▶ Cannot solve the request because of: No candidates
      were found for jaxlib cuda.*.

Copy link
Member

Choose a reason for hiding this comment

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

I see, cc @traversaro

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess we actually need CUDA 12 for the newest version of jaxlib, but apart from that, the modified pyproject works correctly even without forcing the GPU version of jaxlib

Copy link
Member

Choose a reason for hiding this comment

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

Last attempt :) If this does not work, no problem. Possible future errors in GPU resolution will be temporary.

diff --git a/pyproject.toml b/pyproject.toml
index 0699a2c..5298be2 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -206,10 +206,14 @@ pytest-icdiff = "*"
 robot_descriptions = "*"
 
 [tool.pixi.feature.gpu]
-dependencies = { cuda-version = "12.*", cuda-cupti = "*" }
 platforms = ["linux-64"]
 system-requirements = { cuda = "12.1" }
 
+[tool.pixi.feature.gpu.dependencies]
+cuda-version = "12.*"
+cuda-cupti = "*"
+jaxlib = { version = "*", build = "*cuda*" }
+
 [tool.pixi.pypi-dependencies]
 jaxsim = { path = "./", editable = true }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This works, thanks a lot! Addressed in aa532e2

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if pixi in this case expects cuda to be installed system-wide or it installs it from conda-forge.

As for conda, the driver part of cuda should be installed system-wide (and have a newer version w.r.t. to the cuda version used), while the cuda libraries are installed by conda-forge.

@flferretti
Copy link
Collaborator Author

flferretti commented Aug 21, 2024

Looks good, thanks @flferretti! By chance, did you manage to test this action on your fork as discussed f2f? If not, no worries, we can merge and fix it upstream in a new PR.

Thanks @diegoferigo! Please check flferretti#14

@diegoferigo
Copy link
Member

Looks good, thanks @flferretti! By chance, did you manage to test this action on your fork as discussed f2f? If not, no worries, we can merge and fix it upstream in a new PR.

Thanks @diegoferigo! Please check flferretti#12

Awesome!

flferretti and others added 3 commits August 21, 2024 13:04
Co-authored-by: Diego Ferigo <diego.ferigo@iit.it>
Co-authored-by: Diego Ferigo <diego.ferigo@iit.it>
@diegoferigo
Copy link
Member

diegoferigo commented Aug 21, 2024

No need to say, @flferretti trigger a pixi.lock update after merging :) Let's try to remember to do it before tagging releases (please remind me 😄).

@flferretti flferretti merged commit 8e1324b into main Aug 21, 2024
24 checks passed
@flferretti flferretti deleted the 199-add-a-scheduled-workflow-to-automatically-update-the-pixilock-file branch August 21, 2024 11:59
@flferretti
Copy link
Collaborator Author

No need to say, @flferretti trigger a pixi.lock update after merging :) Let's try to remember to do it before tagging releases (please remind me 😄).

We could set that in workflow_dispatches

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.

Add a scheduled workflow to automatically update the pixi.lock file
3 participants