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

[WIP] 8134 Add unit test for realtime inference #8146

Open
wants to merge 13 commits into
base: dev
Choose a base branch
from

Conversation

Nic-Ma
Copy link
Contributor

@Nic-Ma Nic-Ma commented Oct 12, 2024

Fixes #8134 .

Description

This PR added unit test to cover the realtime inference with bundles.
And updated BundleWorkflow to support cyclically calling the run function with all components instantiated.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@ericspod
Copy link
Member

Hi @Nic-Ma, we haven't had time to discuss the ideas around realtime/streaming inference yet due to MICCAI, but I feel we should look into doing this with specialised versions of datasets and transforms to represent streams of incoming and outgoing items. This would implement streaming without modifying existing workflows if we can get it performant.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Oct 13, 2024

Hi @ericspod ,

Yes, I totally agree with you, I was thinking both the streaming way and repeatedly calling way.
Actually, for these 2 calling methods:
(1) pending IterableDataset in a separate thread for the real-time inference input streaming data.
(2) Repeatedly call BundleWorkflow.run() with replaced input data, just what I made the simple test case in this PR.

This PR is just a draft implementation for discussion, could you please help also write draft code for your idea and let's discuss later?
When you design the logic, please consider below typical use cases:
(1) Triton backend: https://github.com/triton-inference-server/python_backend?tab=readme-ov-file#usage
(2) MONAI deploy: https://github.com/Project-MONAI/monai-deploy-app-sdk/blob/main/monai/deploy/operators/monai_bundle_inference_operator.py#L277
(3) Holoscan inference: https://github.com/nvidia-holoscan/holohub/tree/main/tutorials/dicom_to_usd_with_monai_and_holoscan#monai-deploy--holoscan-pipeline

Thanks in advance.

@@ -192,6 +192,16 @@ def get_resolved_content(self, id: str, **kwargs: Any) -> ConfigExpression | str
"""
return self._resolve_one_item(id=id, **kwargs)

def remove_resolved_content(self, id: str) -> Any | None:
Copy link
Member

Choose a reason for hiding this comment

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

This may be a good addition regardless of the intent here.

for i in range(len(self.parser[id])):
sub_id = f"{id}{ID_SEP_KEY}{i}"
ret.append(self.parser.get_parsed_content(sub_id, **kwargs))
self.parser.ref_resolver.remove_resolved_content(sub_id)
Copy link
Member

Choose a reason for hiding this comment

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

This prevents keeping large objects created by the expression evaluation which aren't used anymore, so might be an optimisation in some cases.

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.

Add responsive inference support in bundle
2 participants