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

Some improvements on the YarpRobotLoggerDevice and removed ROS1 device #910

Merged
merged 17 commits into from
Jan 20, 2025

Conversation

S-Dafarra
Copy link
Member

@S-Dafarra S-Dafarra commented Nov 13, 2024

I did the following:

  • Fixed a warning about missing metadata indicating also the channel name
  • Added the possibility of disabling the text logging
  • Worked on the logic behind the error message "the video thread spent more time..." (it appeared to be continuous after the first occurrence)
  • Added info messages when saving the data, indicating how much time it took

@S-Dafarra S-Dafarra force-pushed the improvements_logger branch from c307587 to 393f356 Compare January 10, 2025 13:42
@S-Dafarra S-Dafarra marked this pull request as ready for review January 10, 2025 16:16
@GiulioRomualdi
Copy link
Member

The CI failed because of a deprecated class https://github.com/ami-iit/bipedal-locomotion-framework/blob/ec8a8cf6ac048e3b906a47d71ce9458895bc454b/src/YarpUtilities/include/BipedalLocomotion/YarpUtilities/RosPublisher.h that is not supported anymore.

If you agree we can remove that class since no one is using it (as far as I know)

@S-Dafarra
Copy link
Member Author

The CI failed because of a deprecated class ec8a8cf/src/YarpUtilities/include/BipedalLocomotion/YarpUtilities/RosPublisher.h that is not supported anymore.

If you agree we can remove that class since no one is using it (as far as I know)

I'll do it!

@S-Dafarra S-Dafarra changed the title Some improvements on the YarpRobotLoggerDevice Some improvements on the YarpRobotLoggerDevice and removed ROS1 device Jan 13, 2025
@S-Dafarra
Copy link
Member Author

The CI failed because of a deprecated class ec8a8cf/src/YarpUtilities/include/BipedalLocomotion/YarpUtilities/RosPublisher.h that is not supported anymore.
If you agree we can remove that class since no one is using it (as far as I know)

I'll do it!

I have removed it with a81da33

Now there are other issues due to the fact the iCubGazeboV3 is no more available in icub-models and the BaseEstimatorFromFootIMU fails with the iCub2.5 model

@S-Dafarra
Copy link
Member Author

On Windows all the python bindings tests are failing with the error

___ ERROR collecting bindings/python/System/tests/test_variables_handler.py ___
ImportError while importing test module 'D:\a\bipedal-locomotion-framework\bipedal-locomotion-framework\bindings\python\System\tests\test_variables_handler.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\importlib\__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
..\bindings\python\System\tests\test_variables_handler.py:4: in <module>
    import bipedal_locomotion_framework.bindings.system as blf
bipedal_locomotion_framework\__init__.py:8: in <module>
    from .bindings import *
E   ImportError: DLL load failed while importing bindings: The specified module could not be found.

It seems the same error that should have been fixed with #905

@S-Dafarra
Copy link
Member Author

S-Dafarra commented Jan 13, 2025

I was able to reproduce it on my machine.
First of all, I noticed that the bindings.blablabla.pyd was generated in the bipedal_locomotion_framework\Release subfolder. To avoid appending the Release folder, I used a generator in 4d86cef#diff-f6e0feaeb29fd9f0d3e70bcbac687d9d00341843204bae95b86e8a50fbb00d10R52 as suggested in https://cmake.org/cmake/help/latest/prop_tgt/LIBRARY_OUTPUT_DIRECTORY.html

I was still having issues, but somehow they disappeared when I added help("modules") in the __init__.py file. In the end, a workaround was to import idyntree before importing the bindings. My hunch is that if we don't load a dependency first via python, the dll_directory containing the libraries is not added. Hence, when we try to import the blf's dll, it is not able to find the dependencies' libraries, because the corresponding directory has not yet been added with add_dll_directory.
I did this with 4d86cef#diff-566e805f30ebf7c2568674cc76b3ecfa1dd3c2dbffd7d004d16dc58fe9b6736fR72-R74

Finally, the __init__.py was growing indefinitely. Fixed with 4d86cef#diff-566e805f30ebf7c2568674cc76b3ecfa1dd3c2dbffd7d004d16dc58fe9b6736fR53

cc @traversaro

@S-Dafarra
Copy link
Member Author

I was still having issues, but somehow they disappeared when I added help("modules") in the __init__.py file. In the end, a workaround was to import idyntree before importing the bindings. My hunch is that if we don't load a dependency first via python, the dll_directory containing the libraries is not added. Hence, when we try to import the blf's dll, it is not able to find the dependencies' libraries, because the corresponding directory has not yet been added with add_dll_directory.
I did this with 4d86cef#diff-566e805f30ebf7c2568674cc76b3ecfa1dd3c2dbffd7d004d16dc58fe9b6736fR72-R74

This did not work on CI (🥲), but interestingly now it fails importing idyntree:

___ ERROR collecting bindings/python/System/tests/test_variables_handler.py ___
ImportError while importing test module 'D:\a\bipedal-locomotion-framework\bipedal-locomotion-framework\bindings\python\System\tests\test_variables_handler.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\importlib\__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
..\bindings\python\System\tests\test_variables_handler.py:4: in <module>
    import bipedal_locomotion_framework.bindings.system as blf
bipedal_locomotion_framework\__init__.py:8: in <module>
    import idyntree
C:\Miniconda3\envs\test\Lib\site-packages\idyntree\__init__.py:8: in <module>
    from . import swig
C:\Miniconda3\envs\test\Lib\site-packages\idyntree\swig.py:10: in <module>
    from . import _iDynTree
E   ImportError: DLL load failed while importing _iDynTree: The specified module could not be found.

The difference with my setup is that here idyntree is installed via conda, while in my setup it is installed in the superbuild. I suppose that we should add the conda path too.

@S-Dafarra
Copy link
Member Author

I tried on my laptop, but I was not able to replicate the issue

@traversaro
Copy link
Collaborator

The strange thing from the CI is the following:

C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\importlib\__init__.py:127: in import_module

It seems that is not using the conda python, and that could explain the problem.

@S-Dafarra
Copy link
Member Author

S-Dafarra commented Jan 14, 2025

The strange thing from the CI is the following:

C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\importlib\__init__.py:127: in import_module

It seems that is not using the conda python, and that could explain the problem.

Ah interesting! I also noted that this line is not executed


in fact, icub-models is still present https://github.com/ami-iit/bipedal-locomotion-framework/actions/runs/12752900399/job/35543338419?pr=910#step:8:78

I wonder if it is due to the output of the conda activation that triggers an early termination of the shell

@traversaro
Copy link
Collaborator

Yes, the problem is as early as the CMake configuration, where:

2025-01-13T17:47:34.8240527Z -- Found Python3: C:/hostedtoolcache/windows/Python/3.9.13/x64/python3.exe (found version "3.9.13") found components: Interpreter

@traversaro
Copy link
Collaborator

I opened #920 to debug without polluting this PR.

@traversaro
Copy link
Collaborator

I am not sure what is happening. For sure this is related to a long standing Python on Windows issue:

In a nutshell, for historical reasons official Python binaries and conda binaries of Python on Windows only install python.exe, while Linux and macOS installation contains both python and python3 executables.

If the problem was just that, it would be not particularly problematic, as everyone could just use python as the entrypoint. However, the major problem is that most Linux distributions (due to a legacy of the python2 --> python3 migration) do not ship the python executable by default (for example on ubuntu you need to install the python-is-python3 package). So, effectively there is no command that you are sure that will always be available on all python installation.

Anyhow, this was never a problem until now, as CMake had always found the correct (conda) python, even if GitHub Actions installed a python3.exe executable (that so is not "hidden" by any conda executable) in C:\hostedtoolcache\windows\Python. However, apparently something changed recently at some level (CMake? Python? blf-specific?) and this is not working anymore, but I was not able to investigate more (even as everything is still working fine in other repos's CI like idyntree).

Anyhow, I added a workaround in d7d8fd3 that basically explicitly tell to CMake to use the conda's Python, that should work. I am quite sure we will see this problem again somewhere else, but at least this will unblock the PR here.

@traversaro
Copy link
Collaborator

I am not sure what is happening. For sure this is related to a long standing Python on Windows issue:

* [Windows python3 executable python/cpython#99185](https://github.com/python/cpython/issues/99185)

* [Add entry point for python3 command conda-forge/python-feedstock#349](https://github.com/conda-forge/python-feedstock/issues/349)

In a nutshell, for historical reasons official Python binaries and conda binaries of Python on Windows only install python.exe, while Linux and macOS installation contains both python and python3 executables.

If the problem was just that, it would be not particularly problematic, as everyone could just use python as the entrypoint. However, the major problem is that most Linux distributions (due to a legacy of the python2 --> python3 migration) do not ship the python executable by default (for example on ubuntu you need to install the python-is-python3 package). So, effectively there is no command that you are sure that will always be available on all python installation.

Anyhow, this was never a problem until now, as CMake had always found the correct (conda) python, even if GitHub Actions installed a python3.exe executable (that so is not "hidden" by any conda executable) in C:\hostedtoolcache\windows\Python. However, apparently something changed recently at some level (CMake? Python? blf-specific?) and this is not working anymore, but I was not able to investigate more (even as everything is still working fine in other repos's CI like idyntree).

Anyhow, I added a workaround in d7d8fd3 that basically explicitly tell to CMake to use the conda's Python, that should work. I am quite sure we will see this problem again somewhere else, but at least this will unblock the PR here.

Obviously this was not enough. After passing -DPython3_EXECUTABLE to the main cmake invocation, the cmake invocation inside the cmake-package-check cli tool failed with error:

-- Found Python3: C:/hostedtoolcache/windows/Python/3.12.8/x64/python3.exe (found version "3.12.8") found components: Interpreter
Traceback (most recent call last):
  File "C:\Miniconda3\envs\test\Library\share\ament_cmake_core\cmake\package_templates\templates_2_cmake.py", line 21, in <module>
    from ament_package.templates import get_environment_hook_template_path
ModuleNotFoundError: No module named 'ament_package'
CMake Error at C:/Miniconda3/envs/test/Library/share/ament_cmake_core/cmake/ament_cmake_package_templates-extras.cmake:41 (message):
  execute_process(C:/hostedtoolcache/windows/Python/3.12.8/x64/python3.exe
  C:/Miniconda3/envs/test/Library/share/ament_cmake_core/cmake/package_templates/templates_2_cmake.py
  C:/Users/runneradmin/AppData/Local/Temp/tmpq2a7nqew/ament_cmake_package_templates/templates.cmake)
  returned error code 1
Call Stack (most recent call first):
  C:/Miniconda3/envs/test/Library/share/ament_cmake_core/cmake/ament_cmake_coreConfig.cmake:41 (include)
  C:/Miniconda3/envs/test/Library/share/rclcpp/cmake/ament_cmake_export_include_directories-extras.cmake:8 (find_package)
  C:/Miniconda3/envs/test/Library/share/rclcpp/cmake/rclcppConfig.cmake:41 (include)
  C:/Miniconda3/envs/test/Library/cmake/BipedalLocomotionFrameworkConfig.cmake:28 (find_package)
  CMakeLists.txt:5 (find_package)

Beside the fact that calling python.exe inside a CMake package config file is not a good idea (but that is out of our control, and hopefully CPS will solve all these problems) the strange thing is that in this case the Python found is not even in the PATH.

Anyhow, this for sure is not the end of it, but to unblock the critical BLF CI, in b172916 I did just removed all the C:/hostedtoolcache/windows/Python folder and that should solve everything (for now).

@S-Dafarra
Copy link
Member Author

Awesome, thanks a lot!

@S-Dafarra
Copy link
Member Author

@GiulioRomualdi it might be useful proceeding with this since it fixes several issues for the CI of other PRs.

@S-Dafarra S-Dafarra force-pushed the improvements_logger branch from 106fda2 to 1517627 Compare January 17, 2025 15:17
@S-Dafarra
Copy link
Member Author

Rebased to fix conflicts on Changelog

@GiulioRomualdi GiulioRomualdi merged commit 1721c3e into master Jan 20, 2025
11 of 12 checks passed
@GiulioRomualdi GiulioRomualdi deleted the improvements_logger branch January 20, 2025 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants