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

Finalize minor changes for v0.4 release #186

Merged
merged 29 commits into from
Jul 3, 2024
Merged

Conversation

flferretti
Copy link
Collaborator

@flferretti flferretti commented Jun 21, 2024

This pull request adds minor fixes and updates in preparation for the new release.

@diegoferigo @xela-95 feel free to push similar changes.


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

@diegoferigo
Copy link
Member

Thanks @flferretti for these minor changes.

@traversaro @flferretti I'd like to start discussing again about the lock file of pixi. I'm not happy with its size and the size of each modification that occur when it's re-rendered. As far as I understood, there is no real workaround. My proposal is the following:

  1. Remove the lock file from the repo.
  2. Include the rendered lock file as release artifact, possibly automatically generated from CI.
  3. Add in CI a new test that execute the examples from the resulting environment.

The two benefits we get from this approach is that:

  1. We avoid large diffs in our git repo and a constantly growing size with little benefit.
  2. We don't have to re-render manually the lock file once in a while.
  3. Instead of ensuring that the examples from the latest commit can run on a known set of dependencies, we ensure a similar but more loose property on just the releases.

In general, if users clone the repository directly, the intended usage is creating the environment using the environment.yml, not pixi.lock. Any comment?

@traversaro
Copy link
Contributor

Ideally the nice thing of having the pixi.lock in the repo is to ensure all the users and CI have the same version when developing (but this assumes that they all use pixi, at least until prefix-dev/pixi#1427 is complete), but clearly these needs to be considered as tradeoff with the other downsides like size. We can discuss this in person next week also with @flferretti ? I wonder if it could make sense to just register pixi.lock as a git lfs file? While git lfs is typically used for non-binary files, I think it may be a good fit for pixi.lock files, and there would be no need for users to actually enable git lfs unless they want to use pixi.

@diegoferigo
Copy link
Member

Ideally the nice thing of having the pixi.lock in the repo is to ensure all the users and CI have the same version when developing (but this assumes that they all use pixi, at least until prefix-dev/pixi#1427 is complete), but clearly these needs to be considered as tradeoff with the other downsides like size. We can discuss this in person next week also with @flferretti ? I wonder if it could make sense to just register pixi.lock as a git lfs file? While git lfs is typically used for non-binary files, I think it may be a good fit for pixi.lock files, and there would be no need for users to actually enable git lfs unless they want to use pixi.

Sure, we can align f2f to make a decision. I like the git lfs idea as it is a opt-in feature for the fraction of users (arbitrarily small or large) that want to use pixi.

@diegoferigo
Copy link
Member

@flferretti for the time being, can you remove the pixi lock file from this PR (with a force push, not a commit that reverts the modification). We will handle the lock file separately in a new PR after our f2f discussion.

@flferretti flferretti force-pushed the prepare_release_v04 branch 5 times, most recently from 510a825 to 15564dd Compare June 26, 2024 16:32
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.

Early review on this draft PR. Some comment on patterns used until now in JaxSim:

  • When jax.typing.ArrayLike was used, it was mainly due my lazyness to specialize it to e.g. jtp.Float|jtp.Vector|jtp.Matrix. The Like include plain python types and some more. If we are sure that it's going to be a jax numpy array, let's use the non-Like versions. Note that it's long time I'm also planning to remove the *Jax types in favour of the non-Like ones.
  • If not necessary because it saves multiple conversions to jaxlie types, let's try to use jaxsim.math.* resources in our codebase.

I'll add some commit as soon as you are done. Thanks!

src/jaxsim/integrators/common.py Outdated Show resolved Hide resolved
src/jaxsim/integrators/common.py Outdated Show resolved Hide resolved
src/jaxsim/integrators/common.py Outdated Show resolved Hide resolved
src/jaxsim/integrators/common.py Outdated Show resolved Hide resolved
src/jaxsim/integrators/common.py Outdated Show resolved Hide resolved
src/jaxsim/integrators/variable_step.py Outdated Show resolved Hide resolved
src/jaxsim/integrators/variable_step.py Outdated Show resolved Hide resolved
src/jaxsim/integrators/variable_step.py Outdated Show resolved Hide resolved
tests/test_automatic_differentiation.py Outdated Show resolved Hide resolved
src/jaxsim/api/data.py Outdated Show resolved Hide resolved
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, last minor comments. Let me know when you're done and I might add last minor changes.

src/jaxsim/api/model.py Outdated Show resolved Hide resolved
src/jaxsim/parsers/descriptions/link.py Outdated Show resolved Hide resolved
src/jaxsim/parsers/rod/utils.py Outdated Show resolved Hide resolved
src/jaxsim/math/__init__.py Outdated Show resolved Hide resolved
@diegoferigo
Copy link
Member

@flferretti I pushed few changes, let me know if you have any comment. Otherwise this PR on my side is ready.

@flferretti
Copy link
Collaborator Author

Thanks @diegoferigo, I'll wait for you to add the fixes for the CI and then I'll change the target branch of #187 to main

@diegoferigo diegoferigo marked this pull request as ready for review July 2, 2024 08:08
@flferretti flferretti merged commit 65ed0c9 into main Jul 3, 2024
29 checks passed
@flferretti flferretti deleted the prepare_release_v04 branch July 3, 2024 08:49
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.

3 participants