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

Fix Python bindings for TensorMemory #655

Merged

Conversation

dagardner-nv
Copy link
Contributor

@dagardner-nv dagardner-nv commented Jan 30, 2023

  • Makes the Python API compatible with the Python C++ bindings for TensorMemory, InferenceMemory & ResponseMemory classes.
  • Replaces the tensors attribute for memory classes with explicit get_tensors and set_tensors methods.
  • Make get_input, set_input, get_output & set_output methods actual methods on the base
  • Associated interface proxy classes now inherit from each other limiting redundant python conversion code.
  • Expose MultiTensorMessage class to Python allowing Python inheritance hierarchy to match that of C++, and consolidating some duplicated code.

The reason for removing the attribute is that on the C++ side returning a Python representation of a tensor is rather costly and is always returned as a copy. We want to avoid the obvious bugs that can occur with anyone doing:

m = tensor_memory.TensorMemory(10)
m.tensors['c'] = cp.zeros(count)

Which would have worked when C++ execution is disabled, and the old API is implying that it should work. Instead the API is changed to:

m = tensor_memory.TensorMemory(10)
tensors = m.get_tensors()

tensors['c'] = cp.zeros(count)

m.set_tensors(tensors)

fixes #604

@dagardner-nv dagardner-nv added bug Something isn't working non-breaking Non-breaking change 2 - In Progress labels Jan 30, 2023
@dagardner-nv dagardner-nv requested a review from a team as a code owner January 30, 2023 19:06
Copy link
Contributor

@mdemoret-nv mdemoret-nv left a comment

Choose a reason for hiding this comment

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

Looking much better. Still need to resolve the outstanding comments but those should be quick. Approved. Merge once all comments are resolved.

@dagardner-nv
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 9cb5a4d into nv-morpheus:branch-23.03 Mar 7, 2023
rapids-bot bot pushed a commit that referenced this pull request Mar 21, 2023
* Log a deprecation warning from the constructor for `ResponseMemoryProbs` & `MultiResponseMemoryProbs`
* Replace usage of `ResponseMemoryProbs` with `ResponseMemory`
* Replace usage of `MultiResponseProbsMessage` with `MultiResponseMessage`
* `AddClassificationsStage` and `AddScoresStage` now accept an optional `output_name="probs"` constructor argument.
* *breaking* Any custom stages which only return `MultiResponseMemoryProbs` from the `accepted_types` method will fail during pipeline build.
* Includes changes in PR #655


fixes #697

Authors:
  - David Gardner (https://github.com/dagardner-nv)
  - Michael Demoret (https://github.com/mdemoret-nv)

Approvers:
  - Michael Demoret (https://github.com/mdemoret-nv)

URL: #711
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[BUG]: TypeError: morpheus._lib.messages.InferenceMemory: No constructor defined!
2 participants