-
Notifications
You must be signed in to change notification settings - Fork 11
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 pyproject.toml-based pixi support #134
Conversation
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.
This is super cool @traversaro! I have some curiosity, left some comment.
I'm not super happy about the size of the lock file either, but let's see how it evolves. For the time being, do we have any use-case for arm machines? If we want to slim it down, maybe we can leave them out at least for now.
Last curiosity: how should the lock file be updated? Do we have to do it manually once in a while (or better, automate it every one/two weeks)?
pyproject.toml
Outdated
[tool.pixi.dependencies] | ||
coloredlogs = "*" | ||
jax = "*" | ||
jaxlib = "*" | ||
jaxlie = "*" | ||
jax-dataclasses = "*" | ||
pptree = "*" | ||
rod = "*" | ||
typing_extensions = "*" | ||
lxml = "*" | ||
mediapy = "*" | ||
mujoco = "*" | ||
libgz-sim8 = "*" |
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.
Should we specify here the minimum versions as setup.cfg
?
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.
To be honest, I wanted to avoid having that information duplicated. We can probably just add a pip check
somewhere before the tests to check if we are violating the setup.cfg constraints, and only add the constraint if necessary.
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.
That sounds good! I most cases, I guess that the solved environments are new enough.
pyproject.toml
Outdated
[tool.pixi.feature.gpu] | ||
|
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.
[tool.pixi.feature.gpu] | |
[tool.pixi.feature.gpu] |
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.
It seems that there is no change?
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.
Nit, just blank line.
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.
The suggestion is not removing the blank file, probably there is a bug in GitHub's suggestion method.
system-requirements = {cuda = "12.1"} | ||
# cuda-nvcc and cuda-cupti are just a workaround for | ||
# https://github.com/conda-forge/jaxlib-feedstock/pull/241 | ||
dependencies = {cuda-version = "12.*", cuda-nvcc = "*", cuda-cupti = "*", jaxlib = "* *cuda*"} |
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.
If we pin a minimum jaxlib
version in tool.pixi.dependency
, should we also copy it here?
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 guess so.
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.
Ok, I was wondering whether we have to duplicate it if a range is specified.
I added a CI to do it automatically in robotology/whole-body-estimators#178, see also the documentation in prefix-dev/pixi#909 . However, given the size of the |
Thanks for the explanation! Given the youth of the tool, we can consider |
Wow the lock is about 1MB, pretty large file considering that the entire git repository after a couple of years of development is just 2.8 MB 😅 |
Ok, I addressed the reviews. Leaving just |
Having just a |
Perfect 😍
This is amazing! Since when? Last time I checked, |
Since the beginning I guess? |
Ok, probably I overlooked things in the past. I always installed |
Is this PR ready for merging? |
Ready for me. Not super excited of the 200KB of space used, but I think the tradeoff with the added usability is worth it. |
Hopefully this will not explode the repo too much, because the file is diff-able so only changes should be recorded. However, it's still an initial add of 200KB of space for sure. |
Yep I agree, 200KB of a diff-able file is not too bad, especially if compared to the initial one 1MB large. Let's start playing with this real-case example of JaxSim, which is pretty interesting due to the need of GPU environments, and then re-evaluate it in a few months. I'm particularly interested in the definition of a custom command that in a single line creates the CPU/GPU environment and then opens one of the existing jupyter notebooks with the examples. It would make the life of external users interested in playing with JaxSim much easier. |
We are almost there I guess, we need to add |
Just an experiment, as pixi 0.18.0 was released today with some convenient features:
pyproject.toml
, no need for a separatepixi.toml
fileFurthermore, the PR make extensive use of the multiple environments feature (see https://prefix.dev/blog/introducing_multi_env_pixi) to permit to separate run and test dependencies, and separate environments for gpu support (just on
linux-64
). In a nutshell the following 4 environments are available:default
: the default environment, useful to runjaxsim
on CPUtest-cpu
: environment with test dependencies, to testjaxsim
on CPUgpu
: environment with cuda dependencies, to runjaxsim
on CPU or GPUtest-gpu
: environment with test and cuda dependencies, to testjaxsim
on GPUTo check that this is actually working as intended, run
import jax;print(jax.devices())
in the different environments:To run the tests, run:
If you are on machine with CUDA support, a menu will ask if you want to run test in
test-cpu
ortest-gpu
environment, otherwise the test will run automatically intest-cpu
as a fallback.No CI/docs for now as I first need to understand if it is actually useful.
To test this, make sure that you are using the latest pixi 0.18.0 .
I am not super excited by the size of the
pixi.lock
, but I guess it is the price to pay for full reproducibility and the use of multiple environments on multiple operating systems. Probably we can reduce this by removing some operating systems from the platform list.📚 Documentation preview 📚: https://jaxsim--134.org.readthedocs.build//134/