Skip to content

Conversation

@wendell-hom
Copy link
Contributor

Description

Factor out holoscan operators from robotic_ultrasound workflow.

Operators are under holohub/ directory for the time being -- folder name is subject to change.
When tools/env_setup_robot_surgery.sh is run, it will run env_setup/install_clarius_.sh which will trigger the cmake build for holoscan operators and will result in a holohub/build/ and holohub/install/ directory.

To-do:

  • Consider factoring out the ultrasound simulation operators
  • May want to trigger the holoscan operator build from a different location

@wendell-hom wendell-hom self-assigned this Jun 6, 2025
@mingxin-zheng
Copy link
Contributor

/build

@mingxin-zheng mingxin-zheng requested a review from Copilot June 6, 2025 04:31
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors Holoscan operator implementations into a new holohub/ directory and updates the robotic_ultrasound workflow scripts to import and use these external operators. Key changes include:

  • Moving RealsenseOp, ClariusSolumOp, ClariusCastOp, and NoOp into holohub/operators/* with corresponding CMake build recipes.
  • Updating workflow application scripts to import operators from holohub.operators instead of defining them inline.
  • Adding environment setup scripts (install_clarius.sh) and modifying the main setup to build and install the new operator libraries.

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
workflows/.../camera.py Updated imports to holohub.operators, removed inline operator classes, and adjusted flow wiring.
workflows/.../examples/README.md Removed existing README content (now empty).
workflows/.../clarius_solum.py & clarius_cast.py Switched from inline operator definitions to holohub.operators imports.
tools/env_setup/install_clarius.sh & env_setup_robot_us.sh Added new install script for building holohub operator libraries.
holohub/operators/* Added standalone operator implementations and CMakeLists for building them.
Comments suppressed due to low confidence (1)

workflows/robotic_ultrasound/scripts/holoscan_apps/examples/README.md:1

  • The examples/README.md was cleared out and no longer provides any guidance. Consider restoring or updating this documentation so users know how to run the example apps.
-This folder contains the examples to execute the holoscan sensor apps, and connect with the `policy_runner`.

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Jun 6, 2025

Hi @wendell-hom ,

  1. I would suggest to add 1 top-level folder source referring to the IsaacLab project: https://github.com/isaac-sim/IsaacLab/tree/main/source. So it becomes source/holohub. We will also extract the common simulation modules and APIs and put them in source to build package for users to install.
  2. Could you please help also refactor the operators in the Telesurgery workflow? https://github.com/isaac-for-healthcare/i4h-workflows/tree/main/workflows/telesurgery.

@jjomier @mingxin-zheng What do you think?

Thanks.

@wendell-hom
Copy link
Contributor Author

Hi @wendell-hom ,

  1. I would suggest to add 1 top-level folder source referring to the IsaacLab project: https://github.com/isaac-sim/IsaacLab/tree/main/source. So it becomes source/holohub. We will also extract the common simulation modules and APIs and put them in source to build package for users to install.
  2. Could you please help also refactor the operators in the Telesurgery workflow? https://github.com/isaac-for-healthcare/i4h-workflows/tree/main/workflows/telesurgery.

@jjomier @mingxin-zheng What do you think?

Thanks.

@Nic-Ma yes, I'll look into refactoring the operators in telesurgery after getting some feedback on this one

Copy link
Contributor

@jjomier jjomier left a comment

Choose a reason for hiding this comment

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

Are the tests passing?

@wendell-hom
Copy link
Contributor Author

Are the tests passing?

Yes, tests are passing. There is one visualization test, that is outdated and needs Yun to look into. I'll start a thread on that in i4h channel

@wendell-hom
Copy link
Contributor Author

/build

@wendell-hom
Copy link
Contributor Author

/build

Copy link
Contributor

@mingxin-zheng mingxin-zheng left a comment

Choose a reason for hiding this comment

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

Thank you @wendell-hom for the well-structured refactoring that centralizes operator implementations from scattered workflow locations into a single directory. I only had two minor comments above and please feel free to address them (or not)

@wendell-hom
Copy link
Contributor Author

/build

@mingxin-zheng mingxin-zheng enabled auto-merge (squash) June 11, 2025 05:01
@Nic-Ma Nic-Ma disabled auto-merge June 11, 2025 05:19
@Nic-Ma Nic-Ma closed this Jun 11, 2025
@Nic-Ma Nic-Ma reopened this Jun 11, 2025
@wendell-hom
Copy link
Contributor Author

Hi @wendell-hom ,

  1. I would suggest to add 1 top-level folder source referring to the IsaacLab project: https://github.com/isaac-sim/IsaacLab/tree/main/source. So it becomes source/holohub. We will also extract the common simulation modules and APIs and put them in source to build package for users to install.
  2. Could you please help also refactor the operators in the Telesurgery workflow? https://github.com/isaac-for-healthcare/i4h-workflows/tree/main/workflows/telesurgery.

@jjomier @mingxin-zheng What do you think?

Thanks.

@Nic-Ma, I don't think putting holoscan_i4h under source here works as well because it will also have build/ and install/ directories, unless we move build and install outside to the root folder. Then it will be implied that the build/ and install/ folders are for holoscan_i4h (or they are shared between holoscan_i4h and simulation)

i4h-workflows
├── source/
│       ├── holoscan_i4h/
│       └── simulation/
├── build/
├── install/
└── ...

If we don't care so much about having a source folder, then we could do something like the following where holoscan_i4h and simulation can have their own build and install folder and keep them independent. A possible advantage to this might be that it'll be easier to trigger the builds for holoscan side vs simulation side separately.

i4h-workflows
├── holoscan_i4h/
│       ├── build/
│       └── install/
├──  simulation/
│       ├── build/
│       └── install/
└── ...

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Jun 11, 2025

Hi @wendell-hom ,

OK, let's merge without source folder for now.
If we want to extract the the APIs and modules to source in the future, we can do the refactoring together.

Thanks.

@Nic-Ma Nic-Ma merged commit c00b693 into main Jun 11, 2025
7 checks passed
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.

6 participants