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 ControlMessage to hold arbitrary Python objects & update MessageMeta to copy & slice #1637

Merged

Conversation

yczhang-nv
Copy link
Contributor

@yczhang-nv yczhang-nv commented Apr 17, 2024

Description

  • For ControlMessage, use a specialization of nlohmann::basic_json to hold arbitrary Python objects for m_metadata & m_tasks

  • Implement dataframe slicing method for MessageMeta which is equivalent to copy_meta_ranges() in MultiMessage. The method will do copy & slicing to the original dataframe, instead of sharing the ownership.

By Submitting this PR I confirm:

  • I am familiar with the Contributing Guidelines.
  • When the PR is ready for review, new or existing tests cover these changes.
  • When the PR is ready for review, the documentation is up to date with these changes.

@yczhang-nv yczhang-nv self-assigned this Apr 17, 2024
@yczhang-nv yczhang-nv linked an issue Apr 17, 2024 that may be closed by this pull request
2 tasks
@yczhang-nv yczhang-nv linked an issue Apr 17, 2024 that may be closed by this pull request
2 tasks
@yczhang-nv yczhang-nv added non-breaking Non-breaking change improvement Improvement to existing functionality labels Apr 17, 2024
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.

  • set_metadata and get_metadata should return a JSONValues object.
  • Same with Tasks

morpheus/_lib/src/messages/control.cpp Outdated Show resolved Hide resolved
morpheus/_lib/src/messages/control.cpp Outdated Show resolved Hide resolved
morpheus/_lib/src/messages/control.cpp Outdated Show resolved Hide resolved
morpheus/_lib/src/messages/control.cpp Outdated Show resolved Hide resolved
@yczhang-nv yczhang-nv marked this pull request as ready for review May 3, 2024 19:24
@yczhang-nv yczhang-nv requested a review from a team as a code owner May 3, 2024 19:24
@yczhang-nv yczhang-nv changed the title Update ControlMessage to use JsonValue and MessageMeta to copy & slice Update ControlMessage to hold arbitrary Python objects & update MessageMeta to copy & slice May 3, 2024
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 good but lets build on this. Other things we should be thinking of here:

  • How could we possibly break this code? Lets make tests to try and break it and fix the implementation if they succeed. Try different types of objects, overwriting objects, deleting objects, etc.
  • Can we verify that the python object was destroyed if we erase the key that is holding it?
  • What other utility functions would developers want when working with these objects?

morpheus/_lib/include/morpheus/messages/control.hpp Outdated Show resolved Hide resolved
morpheus/_lib/src/messages/control.cpp Outdated Show resolved Hide resolved
tests/messages/test_control_message.py Outdated Show resolved Hide resolved
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.

Update the type casters in include/morpheus/pybind11/json.hpp to use your new type.

morpheus/_lib/include/morpheus/messages/control.hpp Outdated Show resolved Hide resolved
morpheus/_lib/include/morpheus/messages/control.hpp Outdated Show resolved Hide resolved
@mdemoret-nv
Copy link
Contributor

/merge

@rapids-bot rapids-bot bot merged commit 26c95e1 into nv-morpheus:branch-24.06 May 9, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement to existing functionality non-breaking Non-breaking change
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[FEA]: Support JSONValues and slicing dataframe for ControlMessage
2 participants