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

Implement a class to perform inference with the MANN network #652

Merged
merged 4 commits into from
Apr 26, 2023

Conversation

GiulioRomualdi
Copy link
Member

@GiulioRomualdi GiulioRomualdi commented Apr 14, 2023

Before proceeding with the review of the PR I would like to discuss with you (@traversaro @S-Dafarra @paolo-viceconte) how to handle these two facts:

  1. In 7172fa3 I implemented the Findonnxruntime.cmake. I tested only on linux. A possibility is to get onnxruntime with fetchcontent however:
    1. the installation procedure is system dependent. In linux we have to unzip a compressed folder and move the .so and h in the installation folder. In windows via nuget
    2. The installation does not ship any cmake machinery so I don't know how to handle the linking of the libraries in our framework (probably with add_library(<name> <type> IMPORTED [GLOBAL])
  2. In 421da31 I also stored the model for the test. This is pretty heavy (6mb). we may think of storing the model somewhere perhaps in mann-pytorch and take with fetchcontent

@S-Dafarra
Copy link
Member

  1. the installation procedure is system dependent. In linux we have to unzip a compressed folder and move the .so and h in the installation folder. In windows via nuget

I guess we can stick to linux only for the time being

2. In 421da31 I also stored the model for the test. This is pretty heavy (6mb). we may think of storing the model somewhere perhaps in mann-pytorch and take with fetchcontent

That's not desirable. Can't you have a smaller model?

@traversaro
Copy link
Collaborator

Regarding the installation procedure, I would go for:

  • Binary provided by onnxruntime for Linux/apt . I hoped it was static, but even shared will work fine. Discussing with @paolo-viceconte and @GiulioRomualdi I suggested to put this installation on blf, but probably it make more sense to have it on the superbuild.
  • For conda-forge, just install onnxruntime from conda-forge . At the moment onnxruntime is not available on Windows, but for now we can just enable it on Linux/macOS .
  • On vcpkg and homebrew we do not support this, as life is already too short to try to support homebrew and vcpkg.

@traversaro
Copy link
Collaborator

Binary provided by onnxruntime for Linux/apt . I hoped it was static, but even shared will work fine. Discussing with @paolo-viceconte and @GiulioRomualdi I suggested to put this installation on blf, but probably it make more sense to have it on the superbuild.

Done in robotology/robotology-superbuild#1387 .

@GiulioRomualdi
Copy link
Member Author

The PR is finally ready to be reviewed. The model is now downloaded while compiling 🚀

# BSD-3-Clause license.

include_directories(${CMAKE_CURRENT_BINARY_DIR})
configure_file("${CMAKE_CURRENT_SOURCE_DIR}/FolderPath.h.in" "${CMAKE_CURRENT_BINARY_DIR}/MANNModelFolderPath.h" @ONLY)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really a comment for this PR, but we are getting multiple copies of this file. I wonder if we could move this logic within add_bipedal_test. Maybe something to automatically generate this file given a path. Anyhow, this is not blocking at all

src/ML/tests/MANNTest.cpp Outdated Show resolved Hide resolved
src/ML/include/BipedalLocomotion/ML/MANN.h Outdated Show resolved Hide resolved
src/ML/include/BipedalLocomotion/ML/MANN.h Outdated Show resolved Hide resolved
src/ML/include/BipedalLocomotion/ML/MANN.h Outdated Show resolved Hide resolved
src/ML/include/BipedalLocomotion/ML/MANN.h Outdated Show resolved Hide resolved
src/ML/src/MANN.cpp Outdated Show resolved Hide resolved
src/ML/src/MANN.cpp Show resolved Hide resolved
src/ML/src/MANN.cpp Outdated Show resolved Hide resolved
src/ML/src/MANN.cpp Outdated Show resolved Hide resolved
@GiulioRomualdi
Copy link
Member Author

GiulioRomualdi commented Apr 26, 2023

I think I applied all your suggestions, so in theory, the PR could be merged. However, we need ami-iit/mann-pytorch#5 to get merged before this one (@paolo-viceconte)

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