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

Update CUDA stream handler includes and function names #266

Merged

Conversation

gigony
Copy link
Contributor

@gigony gigony commented Apr 3, 2024

The CudaStreamHandler's API has been updated to use underscores, causing warning messages when the old method names are used, starting from version 1.0.

[warning] [cuda_stream_handler.cpp:86] CudaStreamHandler's `fromMessage` method has been renamed to `from_message`. The old name is deprecated and may be removed in a future release.
[warning] [cuda_stream_handler.cpp:224] CudaStreamHandler's `getCudaStream` method has been renamed to `get_cuda_stream`. The old name is deprecated and may be removed in a future release.
...

This pull request updates the CUDA stream handler includes and function names in the lstm_tensor_rt_inference, npp_filter, orsi_format_converter, orsi_segmentation_postprocessor, orsi_segmentation_preprocessor, orsi_visualizer, qt_video, and tool_tracking_postprocessor operators to use the new holoscan/utils/cuda_stream_handler.hpp header and the new function names with underscores.

The changes include:

  1. Replace the include path of the cuda_stream_handler.hpp header from ../utils/cuda_stream_handler.hpp to holoscan/utils/cuda_stream_handler.hpp
  2. Replace the function names of the CudaStreamHandler class with the new ones using underscores, such as defineParams to define_params, fromMessage to from_message, getCudaStream to get_cuda_stream, and toMessage to to_message.
  3. These changes improve the consistency and readability of the CUDA stream handler usage in the operators and align with the new naming conventions of the holoscan/utils/cuda_stream_handler.hpp header.

@gigony gigony force-pushed the update_cuda_stream_handler_calls branch from 2babce8 to 94197e6 Compare April 3, 2024 01:12
Copy link
Contributor

@tbirdso tbirdso left a comment

Choose a reason for hiding this comment

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

Hi @gigony , does this present a backwards compatibility issue with Holoscan <1.0.3? Have you confirmed that each of the updated apps is fully compatible with the Holoscan >=1.0.3 where the naming changes are introduced?

gxf_extensions/utils/cuda_stream_handler.hpp Outdated Show resolved Hide resolved
@gigony gigony force-pushed the update_cuda_stream_handler_calls branch from 94197e6 to 8496673 Compare April 4, 2024 22:25
@gigony
Copy link
Contributor Author

gigony commented Apr 4, 2024

Hi @gigony , does this present a backwards compatibility issue with Holoscan <1.0.3?

Thanks @tbirdso for the feedback!

I think it wouldn't be backward-compatible with the old version (<1.0.3) because the new methods didn't exist.

Looks like Holohub repository is not versioned (tagged).
I think it's okay to hold off on merging until we can push version-specific commits that break backward compatibility.

Have you confirmed that each of the updated apps is fully compatible with the Holoscan >=1.0.3 where the naming changes are introduced?

I tested with only one or two applications in holohub with this change. (I wonder what is the best way to verify each of the applications locally).

Besides my mistake of removing gxf_extensions/utils/cuda_stream_handler.hpp (as you pointed out), I believe it won't cause a regression as long as the build for existing apps is successful.

@tbirdso
Copy link
Contributor

tbirdso commented Apr 5, 2024

Thanks @gigony !

Looks like Holohub repository is not versioned (tagged).

Correct, each HoloHub project is versioned individually. We do not currently require all projects update to a new Holoscan version at the same time, though we are exploring clearer maintenance guidance for the future.

As a result, unfortunately we would need to determine whether updating each of these affected projects to Holoscan v1.0.3 would introduce regressions.

I wonder what is the best way to verify each of the applications locally.

The current suggested verification procedure for updating a HoloHub project is as follows:

  1. Manually build and test the application locally. This is preferred, but not always possible for projects that require additional hardware or advanced environment setup.
  2. If you can't verify locally, create a dedicated PR for the project changes and tag the project maintainers for verification help.

Unfortunately the current process places a rather large burden on developers when suggesting updates across projects like you've put together here. We're looking into ways that we can streamline and otherwise reduce the burden of this process.

Ideally, I'll suggest the following:

  1. For each affected project, create a new PR with the change suggested here and update the metadata.json minimum and tested Holoscan SDK fields to v1.0.3.
  2. Tag the original project contributors for review and help with validation on each PR.

More practically, if you don't have the time for that right now it's OK to leave this draft open for others to reference as they update their applications to v1.0.3 and later.

@tbirdso
Copy link
Contributor

tbirdso commented Jun 5, 2024

Hi @gigony , thanks for your patience. Based on feedback from @jjomier we can move forward with this change. We expect any app breakage (if any) from these updates to v1.0.3 will be caught in our internal dashboard and fixed at that time.

Would you please confirm whether we are ready to merge, and resolve the merge conflict?

@jjomier
Copy link
Contributor

jjomier commented Jun 24, 2024

@gigony can you look into resolving the conflict?

We don't update the method names in:
   gxf_extensions/utils/cuda_stream_handler.hpp

This is because the method names are used in codelets
(under 'gxf_extensions') and GXF is using camel case for
method names.

Signed-off-by: Gigon Bae <gbae@nvidia.com>
@gigony gigony force-pushed the update_cuda_stream_handler_calls branch from 8496673 to 2c7ef65 Compare August 15, 2024 22:58
@jjomier jjomier merged commit 368309d into nvidia-holoscan:main Aug 15, 2024
2 of 3 checks passed
sohamm17 pushed a commit that referenced this pull request Sep 3, 2024
We don't update the method names in:
   gxf_extensions/utils/cuda_stream_handler.hpp

This is because the method names are used in codelets
(under 'gxf_extensions') and GXF is using camel case for
method names.

Signed-off-by: Gigon Bae <gbae@nvidia.com>
ronyrad pushed a commit to ronyrad/holohub that referenced this pull request Nov 8, 2024
…oloscan#266)

We don't update the method names in:
   gxf_extensions/utils/cuda_stream_handler.hpp

This is because the method names are used in codelets
(under 'gxf_extensions') and GXF is using camel case for
method names.

Signed-off-by: Gigon Bae <gbae@nvidia.com>
Signed-off-by: Rony Rado <rrado@nvidia.com>
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.

3 participants