Add pixi support to build and run IMU test #71
Conversation
martinaxgloria
changed the title
pixi.toml
Outdated
| robometry = ">=1.2.3,<1.3" | ||
| idyntree = ">=10.3.0,<10.4" | ||
| yarp = ">=3.9.0,<3.10" | ||
| robot-testing-framework = ">=2.0.1,<2.1" |
There was a problem hiding this comment.
Just a comment: I know that this is default behavior of pixi add (xref: prefix-dev/pixi#639), but I am not sure if this matches what it would be convenient for us. In particular, I guess that that we want to always use recent version of dependencies when we refresh the lock file. Could it make sense for this reason to switch this constraints to be = "*"?
There was a problem hiding this comment.
Anyhow, this is totally not blocking.
There was a problem hiding this comment.
(Note: I edited the previous comment, that was quite rude due to a typo, sorry about that).
pixi.toml
Outdated
| build_imu_test = {cmd = "cmake -S. -B.build -DICUB_TESTS_COMPILES_IMU_TEST=ON"} | ||
| compile_imu_test = {cmd = "cmake --build .build", depends_on = ["build_imu_test"]} |
There was a problem hiding this comment.
Are you sure that this works? If you are not installing the repo, how does the robottestingframework-testrunner find the tests?
There was a problem hiding this comment.
You're right, obviously this doesn't work if robottestingframework is not installed. In this case, to install the dependencies it would be sufficient to clone and compile them as one or multiple pixi tasks?
There was a problem hiding this comment.
Actually robottestingframework-testrunner will work fine (it is installed by the robot-testing-framework package, see https://conda-metadata-app.streamlit.app/?q=conda-forge%2Flinux-64%2Frobot-testing-framework-2.0.1-hcb278e6_1.conda), what is missing is that you need to install the icub-tests, or let somehow know to RTF how to load the plugins compiled by this project. If I recall correctly, you need to do this by setting some env variable, see https://github.com/robotology/robotology-superbuild/blob/0215a26481e12fb2c46acf54492c2852c88a8e35/doc/environment-variables-configuration.md?plain=1#L29 .
Unfortunatly we have a problem similar to prefix-dev/pixi#826, i.e. if you are running pixi in a shell in which you have a setup.sh sourced (for example the one of a ros installation, or of a robotology-superbuild one), we have cross talking. The proper solution for this is for pixi to run in a isolated environment (see prefix-dev/pixi#289), possibily with some whitelisted env variables, but for the time being we could either document to avoid the |
src/imu/imu.h
Outdated
| * You can manually modify the suites/contexts/icub/test_imu.ini file depending on the parameters of the test. In this case, after compiling, you can run: | ||
| * You can find the parameters involved in the test in suites/contexts/icub/test_imu.ini file. | ||
| * To run the test, you have to install <a href="https://pixi.sh/latest/#installation">pixi</a> and then, after cloning this repo, you can run: | ||
| * | ||
| * Example: robottestingframework-testrunner --suite ../suites/imu.xml | ||
| * pixi run imu_test |
There was a problem hiding this comment.
I would still leave the documentation on how to run the tests without pixi. For example, on the robot setup with the robotology-superbuild installed, somebody would may want to run the test using the versions installed via icub-tests, without the need to manually download the repo. There may other reason for which a user may not want to use pixi, so I think it make sense to leave the general documentation, and the pixi (also to avoid a lock-in on pixi).
There was a problem hiding this comment.
Ok, maybe we could add a README.md in which we could list the two possible installation and usage procedures. In the doxygen doc, instead, we'll keep the "traditional" one, i.e. without pixi
| @@ -18,13 +18,14 @@ | |||
| * | |||
| * The purpose of this test is to evaluate the accuracy of the IMU Euler angles measurements. | |||
| * It takes as input the urdf of the robot and make a comparison between the expected values retrieved from the forward kinematics and the ones read from the IMU itself. | |||
| * The test involves the movements of the joints belonging to the part on which the sensor is mounted. The movements are executed sequentially, traversing from the home position to the lower limit, upper limit and back to the home position for each joint. | |||
| * The test involves the movements of the joints belonging to the part on which the sensors are mounted. | |||
There was a problem hiding this comment.
Looking at the configuration file https://github.com/robotology/icub-tests/blob/devel/suites/contexts/icub/test_imu.ini#L3, it seems that the test requires the existence of the multipleanalogsensorsserver with prefix /${robotname}/alljoints/inertials . Which robots expose this functionality? It could make sense to either list the robots that expose this functionality, or if that is too complicated from the maintenance point of view, just explicitly mention that the robot needs to expose a multipleanalogsensorsserver device with prefix /${robotname}/alljoints/inertials in its yarprobotinterface config files.
There was a problem hiding this comment.
So far, no robots exposed this functionality upstream but I'm going to do this at least for iCubGenova11 and for iCubGazeboV2_* models I guess. We could use them as references to highlight in the documentation that is mandatory to have this MASserver exposed with that port name. Thanks @traversaro!
|
Hi @traversaro, thank you for the review! I'm going to address each comment at a time |
In 736e4f4 I added a sh file in which I set some env variables that usually, if I remember well, are set when the robotology setup.sh is sourced in the bashrc. In this way, for what concerns while: If other env variables could get involved in our case, could we use the same strategy used in robotology/icub-models-generator#263 and set them to empty? |
|
Cool, great! Can you share the output after those improvements? |
Actually, the output is not the expected one. I'll try to explain the problem. export CMAKE_INSTALL_PREFIX=$CONDA_PREFIX
export YARP_DATA_DIRS=$CMAKE_INSTALL_PREFIX/share/yarp:$CMAKE_INSTALL_PREFIX/share/iCub:$PIXI_PROJECT_ROOT/suites
export LD_LIBRARY_PATH=$CMAKE_INSTALL_PREFIX/lib/robottestingframework:$PIXI_PROJECT_ROOT/.build/pluginsTrying to launch the test with
and the test starts despite the warning. Then I repeated the procedure on my Linux machine in which instead the superbuild is installed and the setup.sh is sourced in the bashrc and this time I had:
In this sense, I removed the source of the robotology-superbuild from the bashrc and I did
As you can see, the It seems that the only way to work in a clean pixi environment is to avoid the source of the robotology-superbuild setup.sh |
|
Can you run |
Here you are: cc @traversaro |
Click to read an OT, not really related to this PROT: I totally agree in trying to robustify the pixi activate (at least until we have prefix-dev/pixi#289) a general comment anyhow I would treat the robotology-superbuild's setup.sh and ROS setup.sh exactly as conda or venv environments, and suggest users not to activate them by default. I know this is not what we did in the past, but sometimes when time passes you realize that some stuff can change. However, activating stuff manually is indeed not really convenient, so probably we can also document how to add new "terminal profiles" that permit to create a new vanilla terminal, or a new terminal with robotology-superbuild sources, or a new terminal with a conda/pixi environment with just a click. For example, on Windows terminal this can be done with:
but similar functionality exists also for most macOS or Linux applications. Things are more complex on the robot, but eventually we could think of something also for that. |
I agree. So, for the time being we will go with avoiding the automatic activation of the robotology-superbuild setup.sh (and we document it) for not having cross-talking problems. |
|
Thanks to the help of @traversaro, in 180cc4a I prepended the cc @Nicogene |
|
I think we can also just set |
There was a problem hiding this comment.
Ok for me, for the pixi part I leave the review to @traversaro. The documentation can be refined later on when we will explain this test to the production
Yes, I just added a skeleton but for sure it will be modified with the next integrations. Thanks @Nicogene! |
As per title, this PR adds the
pixisupport to the IMU test. In particular, now the dependencies are handled in thepixi.lockfile. So now, to run the test, it's possible to simply do:or
imu_sim_testif the test would be run in simulation. In this way, a check on the dependencies is done, and the test is compiled and started.For the sake of completeness, I'll report here the output of this command on a clean clone of the repo: