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 workflow_dispatch and schedule events to ci_cd workflow and constraint jax<0.4.25 to be #93

Merged
merged 4 commits into from
Feb 28, 2024

Conversation

traversaro
Copy link
Contributor

@traversaro traversaro commented Feb 27, 2024

Changes:

  • The CI/CD job use a rolling release distribution for dependencies, if I am not wrong. That is fine, but in that case to track regressions it is convenient to build periodically, and permit to trigger CI jobs run manually.
  • Pin jax to be < 0.4.25, as there have current jaxsim does not work fine with jax==0.4.25
  • Revert Run pytest in CI only in PRs that update either sources or tests #80, as we are currently installing dependencies from a rolling release distribution (PyPI) and so it is possible that there is a test failure even if nothing changed in source files. We can introduce something similar if we start using a lockfile of some kind, if we add the logic also to check changes in the lockfile.

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

@traversaro
Copy link
Contributor Author

I also added a step to print pip list and easily compare pip packages used for different jobs.

@flferretti
Copy link
Collaborator

flferretti commented Feb 27, 2024

It looks good to me, thanks! Yet, I believe that activating a dependabot might be useful for this aim. What do you think about that?

@traversaro
Copy link
Contributor Author

It looks good to me, thanks! Yet, I believe that activating a dependabot might be useful for this aim. What do you think about that?

I guess dependabot needs to work on some kind of lockfile or similar used for testing. Probably we need to add something similar before adding dependabot? One thing that I would pay attention to is to avoid conflating "presumed compatibiltiy ranges of dependencies" and "version of the dependencies used for tests".

@flferretti
Copy link
Collaborator

I guess dependabot needs to work on some kind of lockfile or similar used for testing. Probably we need to add something similar before adding dependabot? One thing that I would pay attention to is to avoid conflating "presumed compatibiltiy ranges of dependencies" and "version of the dependencies used for tests".

Yes, it makes sense. Let me cc @diegoferigo

@traversaro traversaro changed the title Add workflow_dispatch and schedule events to ci_cd workflow Add workflow_dispatch and schedule events to ci_cd workflow and constraint jax<0.4.25 to be Feb 27, 2024
@traversaro
Copy link
Contributor Author

Cool, anyhow it seems that tests seem to work fine with jax==0.4.24, I guess this is a good enough solution to fix the CI, we can enable back compatibility with jax==0.4.25 once we migrate tests to the functional interface.

@flferretti flferretti merged commit 389c556 into ami-iit:main Feb 28, 2024
11 checks passed
@diegoferigo
Copy link
Member

diegoferigo commented Feb 28, 2024

The conditional logic to run pytest was to reduce CI time on PRs that were not touching any .py file. They were running in all other non-PR events. I don't see why the CI of a PR running on non-.py should fail if dependencies are out of sync. In the previous behavior, CI on the merged commit would fail, and that's what I would expect.

Any other reason to remove previous logic? I still don't get the motivation. For the other changes, I agree on both the dispatch and the pinning.

@diegoferigo
Copy link
Member

diegoferigo commented Feb 28, 2024

Ahn ok I see, you were maybe referring to changes of setup.cfg? It could have made sense adding also that file to the exclusion logic alongside the .py files.

@flferretti
Copy link
Collaborator

Ahn ok I see, you were maybe referring to changes of setup.cfg? It could have made sense adding also that file to the exclusion logic alongside the .py files.

Yes, and also to check if using the latest versions of the dependencies make the build fail, like in this case

@diegoferigo
Copy link
Member

Yes but the failure is not due to the changes of this PR specifically, I'd expect to see CI green here. I expect a new PR to fix the problem if the CI of main fails due to new dependencies.

@traversaro
Copy link
Contributor Author

traversaro commented Feb 28, 2024

Yes but the failure is not due to the changes of this PR specifically, I'd expect to see CI green here. I expect a new PR to fix the problem if the CI of main fails due to new dependencies.

The problem we experienced is that the tests also depends on:

  • .github/workflows/ci_cd.yaml
  • pyproject.toml
  • setup.cfg

Then idea is that if you open a PR to check if the CI works, it is a bit counterintuitive if the CI is green even if the test did not run. However, then we added also the schedule and workflow_dispatch triggers to detect this, so indeed we can add back this check if you think it is worth.

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