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 casadi-matlab-bindings and remove qpOASES dependency in whole-body-controllers #783

Closed
wants to merge 1 commit into from

Conversation

traversaro
Copy link
Member

@traversaro traversaro commented Jun 3, 2021

Related to #782 . This ensure that casadi-matlab-bindings are installed also when whole-body-controllers is installed via conda binary packages, as it happens for the MATLAB/Simulink one line installation process (https://github.com/robotology/robotology-superbuild/blob/master/doc/matlab-one-line-install.md) @DanielePucci .

Note that the problem fixed by this PR and described in #782 does not affect running controllers that do not use CasADi, such as the YOGA++ .

@traversaro traversaro changed the title Add casadi-matlab-bindings and remove qpOASES dependecy in whole-body-controllers Add casadi-matlab-bindings and remove qpOASES dependency in whole-body-controllers Jun 3, 2021
@traversaro
Copy link
Member Author

This turns out to be more problematic that expected, as it basically makes casadi and casadi-matlab-bindings a software that is installed when ROBOTOLOGY_USES_DYNAMICS is enabled, and not only when ROBOTOLOGY_USES_DYNAMICS_ADDITIONAL_DEPS is enabled. In particular, now Debian CI fails with:

2021-06-03T09:50:10.5544018Z CMake Deprecation Warning at CMakeLists.txt:31 (cmake_policy):
2021-06-03T09:50:10.5544936Z   The OLD behavior for policy CMP0011 will be removed from a future version
2021-06-03T09:50:10.5545569Z   of CMake.
2021-06-03T09:50:10.5545966Z 
2021-06-03T09:50:10.5546803Z   The cmake-policies(7) manual explains that the OLD behaviors of all
2021-06-03T09:50:10.5547735Z   policies are deprecated and that a policy should be set to OLD only under
2021-06-03T09:50:10.5548889Z   specific short-term circumstances.  Projects should be ported to the NEW
2021-06-03T09:50:10.5549725Z   behavior and not rely on setting a policy to OLD.
2021-06-03T09:50:10.5550149Z 
2021-06-03T09:50:10.5550388Z 
2021-06-03T09:50:10.5551001Z CMake Deprecation Warning at CMakeLists.txt:35 (cmake_policy):
2021-06-03T09:50:10.5551911Z   The OLD behavior for policy CMP0005 will be removed from a future version
2021-06-03T09:50:10.5552546Z   of CMake.
2021-06-03T09:50:10.5552820Z 
2021-06-03T09:50:10.5553652Z   The cmake-policies(7) manual explains that the OLD behaviors of all
2021-06-03T09:50:10.5554555Z   policies are deprecated and that a policy should be set to OLD only under
2021-06-03T09:50:10.5555712Z   specific short-term circumstances.  Projects should be ported to the NEW
2021-06-03T09:50:10.5556553Z   behavior and not rely on setting a policy to OLD.
2021-06-03T09:50:10.5556969Z 
2021-06-03T09:50:10.5557211Z 
2021-06-03T09:50:10.5557776Z CMake Error at CMakeLists.txt:99 (enable_language):
2021-06-03T09:50:10.5558581Z   The Ninja generator does not support Fortran using Ninja version
2021-06-03T09:50:10.5559082Z 
2021-06-03T09:50:10.5559416Z     1.8.2
2021-06-03T09:50:10.5559664Z 
2021-06-03T09:50:10.5560280Z   due to lack of required features.  Ninja 1.10 or higher is required.
2021-06-03T09:50:10.5560776Z 
2021-06-03T09:50:10.5560996Z 
2021-06-03T09:50:10.5561709Z -- Configuring incomplete, errors occurred!

@traversaro
Copy link
Member Author

Another problem that emerged is:

2021-06-03T09:59:14.5923707Z [208/328] Performing build step for 'casadi-matlab-bindings'
2021-06-03T09:59:14.5928431Z ninja: no work to do.
2021-06-03T09:59:14.6386109Z [209/328] Performing install step for 'casadi-matlab-bindings'
2021-06-03T09:59:14.6391377Z FAILED: src/casadi-matlab-bindings/CMakeFiles/YCMStamp/casadi-matlab-bindings-install 
2021-06-03T09:59:14.6399405Z cd /__w/robotology-superbuild/robotology-superbuild/build/src/casadi-matlab-bindings && /usr/bin/cmake --build . --target install && /usr/bin/cmake -E touch /__w/robotology-superbuild/robotology-superbuild/build/src/casadi-matlab-bindings/CMakeFiles/YCMStamp/casadi-matlab-bindings-install
2021-06-03T09:59:14.6406536Z ninja: error: unknown target 'install', did you mean 'uninstall'?

This is happening because casadi-matlab-bindings if ROBOTOLOGY_USES_MATLAB is set to OFF does not install anything. I do not know if the best thing to do is to not add the dependency if ROBOTOLOGY_USES_MATLAB is set to OFF, or to install something dummy in casadi-matlab-bindings.

@traversaro
Copy link
Member Author

traversaro commented Jun 3, 2021

This turns out to be more problematic that expected, as it basically makes casadi and casadi-matlab-bindings a software that is installed when ROBOTOLOGY_USES_DYNAMICS is enabled, and not only when ROBOTOLOGY_USES_DYNAMICS_ADDITIONAL_DEPS is enabled. In particular, now Debian CI fails with:

I think I will need a quick feedback on this from actual users. Brief recap: whole-body-controllers now depends on casadi-matlab-bindings and casadi, that however for now are just part of ROBOTOLOGY_USES_DYNAMICS_ADDITIONAL_DEPS, not of ROBOTOLOGY_USES_DYNAMICS. To fix the situation we have some possible options:

  1. Move casadi and casadi-matlab-bindings in ROBOTOLOGY_USES_DYNAMICS (downside: longer build time for anyone using ROBOTOLOGY_USES_DYNAMICS, increase chance compilation errors on platform not tested by CI)
  2. Move whole-body-controllers in ROBOTOLOGY_USES_DYNAMICS_ADDITIONAL_DEPS (note that yarpmotorgui files with position commonly used in the lab as part of demos are part of whole-body-controllers
  3. Just leave things as they are. The downside of this is that users may press play of a whole-body-controllers controller, and it will not work as casadi-matlab-bindings are missing

Any strong opinion on this @lrapetti @S-Dafarra @gabrielenava ?

@S-Dafarra
Copy link
Collaborator

This turns out to be more problematic that expected, as it basically makes casadi and casadi-matlab-bindings a software that is installed when ROBOTOLOGY_USES_DYNAMICS is enabled, and not only when ROBOTOLOGY_USES_DYNAMICS_ADDITIONAL_DEPS is enabled. In particular, now Debian CI fails with:

I think I will need a quick feedback on this from actual users. Brief recap: whole-body-controllers now depends on casadi-matlab-bindings and casadi, that however for now are just part of ROBOTOLOGY_USES_DYNAMICS_ADDITIONAL_DEPS, not of ROBOTOLOGY_USES_DYNAMICS. To fix the situation we have some possible options:

  1. Move casadi and casadi-matlab-bindings in ROBOTOLOGY_USES_DYNAMICS (downside: longer build time for anyone using ROBOTOLOGY_USES_DYNAMICS, increase chance compilation errors on platform not tested by CI)
  2. Move whole-body-controllers in ROBOTOLOGY_USES_DYNAMICS_ADDITIONAL_DEPS (note that yarpmotorgui files with position commonly used in the lab as part of demos are part of whole-body-controllers
  3. Just leave things as they are. The downside of this is that users may press play of a whole-body-controllers controller, and it will not work as casadi-matlab-bindings are missing

Any strong opinion on this @lrapetti @S-Dafarra @gabrielenava ?

It seems to me that option 3 is the most preferable. Is it possible to avoid the controller to crash and burn if Casadi is not present, and eventually throw a meaningful warning?

@traversaro
Copy link
Member Author

It seems to me that option 3 is the most preferable. Is it possible to avoid the controller to crash and burn if Casadi is not present, and eventually throw a meaningful warning?

No idea, @CarlottaSartore do you have any opinion?

@lrapetti
Copy link
Member

lrapetti commented Jun 3, 2021

Any strong opinion on this @lrapetti @S-Dafarra @gabrielenava ?

I don't have a strong opinion on that, but I would go with what @S-Dafarra proposed if doable.

@CarlottaSartore
Copy link

CarlottaSartore commented Jun 3, 2021

I do not see any problem with the approach proposed by @S-Dafarra for the external projects, since it would mean to correctly document the set of the variable ROBOTOLOGY_USES_DYNAMICS_ADDITIONAL_DEPS to true (which already is the case).

Is it possible to avoid the controller to crash and burn if Casadi is not present, and eventually throw a meaningful warning?

This part could be tricky, if I recall correctly, the model does not compile if Casadi is not found. So we should define a way to proceed (check if casadi is present at the initialization phase a throw an error stating to set to true ROBOTOLOGY_USES_DYNAMICS_ADDITIONAL_DEPS? )

I can dig into the issue to find a suitable way to deal with it.

Move casadi and casadi-matlab-bindings in ROBOTOLOGY_USES_DYNAMICS (downside: longer build time for anyone using ROBOTOLOGY_USES_DYNAMICS, increase chance compilation errors on platform not tested by CI)

It seems to me that also this one could be a valuable option, since it seems to me that the use of Casadi in the lab is increasing lately, although the problem of the compilations error could be critical

@nunoguedelha
Copy link
Collaborator

I would also go for option 3 as an immediate solution, for the reasons already mentioned.

I think option 1 (Move casadi and casadi-matlab-bindings in ROBOTOLOGY_USES_DYNAMICS ) is risky, potentially increasing the compilation errors as @CarlottaSartore mentioned. I haven't tried recently, but I remember having issues for building Casadi on the MacOS, where it was less well supported than on other platforms. This should have been improved with Conda...

About option 2: Move whole-body-controllers in ROBOTOLOGY_USES_DYNAMICS_ADDITIONAL_DEPS.
If I understood correctly, the obstacle here is that yarpmotorgui files with position commonly used in the lab as part of demos are part of whole-body-controllers. If those files are not just used by the MATLAB/Simulink controllers in whole-body-controllers this is actually a flaw we need to correct, and the sooner the better. It shouldn't be a problem moving those files to another component compiled with ROBOTOLOGY_USES_DYNAMICS as they are only configuration files.

So in the long (not too long) run, I would go for option 2 after moving the yarpmotorgui files elsewhere.

@nunoguedelha
Copy link
Collaborator

In any case, whatever option is selected, I think the yarpmotorgui files should be moved elsewhere.

@traversaro
Copy link
Member Author

Given the consensus, let's keep things as they are. casadi is effectively an optional runtime dependency of whole-body-controllers, if this indeed cause any confusion, you can clarify it in the docs. @gabrielenava @CarlottaSartore feel free to improve the docs as you see fit.

@traversaro traversaro closed this Jun 25, 2021
@traversaro
Copy link
Member Author

traversaro commented Jun 25, 2021

In any case, whatever option is selected, I think the yarpmotorgui files should be moved elsewhere.

That was the best compromise we were able to find in robotology-legacy/codyco-modules#273 . If there is a new concrete proposal of where to host those files, and both the whole-body-controllers mantainer (@gabrielenava) and the mantainer of the repo (or of the newly created repo) meant to host those files agree with the change, then I don't think there is anything blocking the change.

fyi @lrapetti @nunoguedelha

@traversaro
Copy link
Member Author

Actually I just realized that in any case we want to drop qpOASES dependency?

@traversaro traversaro deleted the traversaro-patch-1 branch June 18, 2022 09:48
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.

5 participants