Fix call to setComTarget in InverseKinematicsNLP

[diegoferigo]

Thekwargs have been removed from the upstream bindings.

[traversaro]

Thekwargs have been removed from the upstream bindings.

Just to understand, this is something happened at the iDynTree level? Do you have a reference on the PR that made this change?

[diegoferigo]

Yep, the culprit is robotology/idyntree#822. SWIG generatesd kwargs only if there is no ambiguity between methods with the same name. All the duplications in iDynTree due to the Span support have the consequence of producing bindings without kwargs. All the existing code that was using kwargs (mainly for readability) gets broken.

[traversaro]

Yep, the culprit is robotology/idyntree#822. SWIG generated kwargs only if there is no ambiguity between methods with the same name. All the duplications in iDynTree due to the Span support have the consequence of producing bindings without kwargs. All the existing code that was using kwargs (mainly for readability) gets broken.

Thanks, I had no idea. For the future I think that if you think that kwargs or other similar functionalities are useful, feel free to add a test for them on iDynTree (as per Beyonce's Rule, see Page 221 of https://abseil.io/resources/swe_at_google.2.pdf) otherwise it is difficult to catch these regression by manual inspection.

[diegoferigo]

Yep, the culprit is robotology/idyntree#822. SWIG generated kwargs only if there is no ambiguity between methods with the same name. All the duplications in iDynTree due to the Span support have the consequence of producing bindings without kwargs. All the existing code that was using kwargs (mainly for readability) gets broken.

Thanks, I had no idea. For the future I think that if you think that kwargs or other similar functionalities are useful, feel free to add a test for them on iDynTree (as per Beyonce's Rule, see Page 221 of https://abseil.io/resources/swe_at_google.2.pdf) otherwise it is difficult to catch these regression by manual inspection.

I think in this case there's nothing final we can do from downstream. This is not the only place where kwargs are used, and personally I always use them when I can because they greatly improve readability and make downstream code fail if upstream changes the arguments or their order (and this is a good thing otherwise upstream changes could get unnoticed).

In the case of SWIG bindings, maybe we should stop using kwargs because upstream changes like these ones break downstream Python code even though C++ APIs are not broken. I will slowly migrate the code I maintain to avoid their usage.

Note that this is a SWIG-only problem. I think that pybind11 handles it better because it forces upstream to select which method to override when there are multiple methods with the same name. SWIG instead binds all of them and tries to select the appropriate overload dynamically (I guess with a logic similar to multiple dispatch).

[traversaro]

I think in this case there's nothing final we can do from downstream.

Yes, exactly for this reasonI mentioned adding tests in iDynTree itself, rather then downstream, to catch this problems before it gets released (even if in this specific case the ship has sailed).

[diegoferigo]

Yes, exactly for this reasonI mentioned adding tests in iDynTree itself, rather then downstream, to catch this problems before it gets released (even if in this specific case the ship has sailed).

Yes I agree with you. In this specific case I think it is more important keep iDynTree uniform, in the sense that there are already dozens of methods that, with the same name, offer the Span and non-Span variants. Now that we know that it's better avoiding using kwargs on SWIG resources, it's just matter of slowly migrating. We don't have much code that is affected.

[diegoferigo]

I'm not sure why the fuel test of the push job is failing with:

[Wrn] [LocalCache.cc:104] Server directory does not exist [/github/home/.ignition/fuel/fuel.ignitionrobotics.org]
[Err] [Filesystem.cc:459] Failed to create directory [/github/home/.ignition/fuel/fuel.ignitionrobotics.org/openrobotics/models/electrical box/3/materials/textures]: Not a directory
[Err] [Zip.cc:152] Error creating directory [/github/home/.ignition/fuel/fuel.ignitionrobotics.org/openrobotics/models/electrical box/3/materials/textures]. Do you have the right permissions?
[Err] [LocalCache.cc:416] Unable to unzip [/github/home/.ignition/fuel/fuel.ignitionrobotics.org/openrobotics/models/electrical box/3/electrical box.zip]
[Err] [utils.cpp:208] Failed to download Fuel model

The pull job is instead working fine. The difference between the two is that push build Dome with colcon while pull uses binaries from the PPA. Maybe there's some recent change that is not yet included in the binaries, but I couldn't find the source (I didn't search thoroughly). a17647d does not suffice.

[diegoferigo]

No luck, marking the test as flaky and merging as soon as CI turns green.

[diegoferigo]

Just found the culprit of the fuel test failure that was disabled in this PR: gazebosim/gz-fuel-tools#183.