-
Notifications
You must be signed in to change notification settings - Fork 1
14 - Support Dynamic Labels #41
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
Conversation
|
@atimin 'max' mode for CDR does not make sense to me. We currently treat each message to be a record in CDR, which would destroy the logic of 'max'. |
There was a problem hiding this 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 adds support for dynamic labels to the reductstore_agent, allowing labels to be extracted from ROS message fields and attached to recorded data. The feature supports three modes: "last" (most recent value), "first" (first value in file), and "max" (maximum value across messages).
Key Changes
- Introduced
LabelStateTrackerclass to track and update dynamic label values based on incoming ROS messages - Added
LabelTopicConfigandLabelModemodels to configure dynamic label behavior per topic - Integrated label tracking into both MCAP and CDR output writers
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| reductstore_agent/dynamic_labels.py | New file implementing the LabelStateTracker class for tracking dynamic label state across messages |
| reductstore_agent/models.py | Added LabelMode enum and LabelTopicConfig model for configuring dynamic labels; added labels field to PipelineConfig |
| reductstore_agent/writer/mcap.py | Integrated label tracker: added parameter, updated messages, and merged dynamic labels with static labels during upload |
| reductstore_agent/writer/cdr.py | Integrated label tracker: added parameter, updated messages in both batch and streaming paths |
| reductstore_agent/writer/factory.py | Instantiates LabelStateTracker when creating writers if labels are configured; minor docstring formatting improvements |
| reductstore_agent/utils.py | Added extract_field() utility function to extract field values from ROS messages |
| README.md | Added documentation for the three dynamic label modes (last, first, max) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
atimin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, there is something to polish. See my comments. I think we can write tests.
atimin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! I have a few minor suggestions, and then we can merge.
Please fill out the PR description.
atimin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect!
Closes #14
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
Feature - Support Dynamic Labels
What was changed?
Related issues
(Add links to related issues)
Does this PR introduce a breaking change?
(What changes might users need to make in their application due to this PR?)
Other information: