-
Notifications
You must be signed in to change notification settings - Fork 285
feat: Add datetime.datetime support in pydict_to_metadata() for metadata forwarding #1329
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
base: main
Are you sure you want to change the base?
Conversation
This commit introduces a new variant, Timestamp, to the Parameter enum in the metadata module. The Timestamp variant utilizes the chrono library to represent timestamps as DateTime<Utc>, enhancing the metadata's capability to handle time-related data.
…metadata and metadata_to_pydict This commit extends the functionality of the metadata module by implementing datetime conversion for Python objects. It checks for datetime instances in the pydict_to_metadata function and converts them to chrono::DateTime<Utc>. Additionally, it ensures that timestamps are returned as native Python datetime.datetime objects in metadata_to_pydict, maintaining UTC timezone awareness. This improves the usability of timestamps in metadata for Python users.
This commit adds the chrono library as a dependency in the Cargo.toml file, enabling improved datetime handling capabilities in the metadata module. This aligns with recent enhancements for datetime conversion in Python objects, ensuring better usability for time-related data.
…timestamps This commit introduces two new methods, get_timestamp and set_timestamp, to the Metadata struct. These methods allow for retrieving and setting timestamp values in the metadata, utilizing the newly added Timestamp variant in the Parameter enum. The implementation ensures that timestamps are parsed and formatted correctly using the chrono library, enhancing the metadata's capability to manage time-related data.
…ization This commit adds the chrono library to the Cargo.toml file with the serde feature enabled. This enhancement supports improved serialization of datetime objects in the metadata module, aligning with recent updates for better datetime handling in the project.
…tion This commit adds support for serializing the Timestamp variant in the log_to_terminal function, converting it to an RFC 3339 formatted string using the chrono library. This enhancement improves the output formatting of timestamps in the CLI, ensuring consistency with the recent updates for datetime handling.
…ct functions This commit cleans up the datetime conversion logic in the pydict_to_metadata and metadata_to_pydict functions. It removes redundant imports and whitespace, ensuring a more streamlined implementation while maintaining the conversion of Python datetime objects to chrono::DateTime<Utc> and vice versa. This enhances code readability and consistency in datetime handling.
…oved readability This commit refines the serialization logic for the Timestamp variant in the log_to_terminal function by enhancing code formatting. The change improves the clarity of the code while maintaining the functionality of converting timestamps to RFC 3339 formatted strings.
This commit updates the Cargo.lock file to include the chrono library as a dependency. This addition supports improved datetime handling capabilities across the project, aligning with recent enhancements in serialization and metadata management.
kou
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.
Thanks for working on this!
Should we overwrite timestamp here if parameters has timestamp?
dora/libraries/message/src/metadata.rs
Lines 27 to 32 in 6f5269b
| Self { | |
| metadata_version: 0, | |
| timestamp, | |
| parameters, | |
| type_info, | |
| } |
apis/python/operator/src/lib.rs
Outdated
| // timestamp() returns seconds since epoch as float | ||
| // Convert to SystemTime first, then to DateTime<Utc> | ||
| let seconds = timestamp_float as i64; | ||
| let nanos = ((timestamp_float - seconds as f64) * 1_000_000_000.0) as u64; |
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.
How about using u32 here to reduce casts for std::time::Duration::new()?
| let nanos = ((timestamp_float - seconds as f64) * 1_000_000_000.0) as u64; | |
| let nanos = ((timestamp_float - seconds as f64) * 1_000_000_000.0) as u32; |
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.
Or we could just use Duration::try_from_secs_f64, no?
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.
@phil-opp Agreed. Duration::try_from_secs_f64() is cleaner. I'll update the code to use it.
| fn get_list_int(self: &Metadata, key: &str) -> Result<Vec<i64>>; | ||
| fn get_list_float(self: &Metadata, key: &str) -> Result<Vec<f64>>; | ||
| fn get_list_string(self: &Metadata, key: &str) -> Result<Vec<String>>; | ||
| fn get_timestamp(self: &Metadata, key: &str) -> Result<String>; |
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.
If we can use C++20, how about using std::chrono::time_point<sd::chrono::utc_clock>?
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.
Thanks for the suggestion! I noticed -std=c++17 in our example build files, but I haven’t confirmed which C++ standard the API bindings actually use. If C++20 is acceptable for the project, I’d be happy to use std::chrono::time_pointstd::chrono::utc_clock.
@phil-opp any thoughts on this?
| let utc_timezone = datetime_module.getattr("timezone")?.getattr("utc")?; | ||
|
|
||
| // Create timezone-aware datetime using fromtimestamp | ||
| let total_seconds = timestamp as f64 + microseconds as f64 / 1_000_000.0; |
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.
Why not use as_secs_f64?
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.
@phil-opp chrono::DateTime doesn't have as_secs_f64().
…y and efficiency This commit simplifies the timestamp conversion logic in the pydict_to_metadata function by utilizing std::time::Duration's try_from_secs_f64 method. This change enhances code readability and ensures accurate handling of both positive and negative timestamps, while maintaining compatibility with the existing datetime handling improvements.
…ency This commit further simplifies the timestamp conversion logic in the pydict_to_metadata function by consolidating the checked_sub method call into a single line. This change enhances code readability while maintaining the existing functionality for handling negative timestamps.
|
Hi @phil-opp , just checking in to see if you had a chance to look at this. No rush — happy to follow up if needed. |
Problem
When forwarding metadata containing
datetime.datetimeobjects throughnode.send_output(), the system would fail with:could not convert type 2026-01-21 05:12:21.992886+00:00
This blocked the opencv-video-capture use case where metadata (including timestamps) needs to be forwarded from input to output events. For example, opencv-video-capture forwards tick's metadata, but
node.send_output()fails when encounteringdatetime.datetimeobjects in the metadata.Solution
1. Added native timestamp support in Rust metadata
Parameter::Timestamp(chrono::DateTime<Utc>)variant to theParameterenum2. Enhanced
pydict_to_metadata()to detect datetime.datetimedatetime.datetimeobjects before the fallback handlerdatetime.datetime→Parameter::Timestamp(chrono::DateTime<Utc>)3. Enhanced
metadata_to_pydict()for round-trip conversionParameter::Timestampback to timezone-aware Pythondatetime.datetimeobjects4. Extended C++ bindings and CLI
Key Implementation Details
The datetime detection happens before the fallback in
pydict_to_metadata(), ensuring datetime objects are properly recognized:// Check if it's a datetime.datetime object
let datetime_module = PyModule::import(value.py(), "datetime")?;
let datetime_class = datetime_module.getattr("datetime")?;
if value.is_instance(datetime_class.as_ref())? {
// Convert datetime.datetime → Parameter::Timestamp
let timestamp_float: f64 = value.call_method0("timestamp")?.extract()?;
// ... conversion logic ...
parameters.insert(key, Parameter::Timestamp(dt))
} else {
// Fallback only for truly unknown types
println!("could not convert type {value}");
parameters.insert(key, Parameter::String(value.str()?.to_string()))
}## Testing
✅ All tests passing with zero "could not convert type" warnings
✅ Edge cases verified:
I've also attached a few screenshots from local testing showing successful forwarding with no conversion errors.
Files Changed
libraries/message/src/metadata.rs- AddedParameter::Timestampvariantapis/python/operator/src/lib.rs- Datetime detection inpydict_to_metadata()and conversion inmetadata_to_pydict()apis/python/operator/Cargo.toml- Added chrono dependencyapis/c++/node/src/lib.rs- C++ API supportapis/c++/node/Cargo.toml- Added chrono dependencybinaries/cli/src/command/topic/echo.rs- Display formattingCargo.lock- Updated dependenciesImpact
datetime.datetimeusage in metadata without workaroundsRelated Issues
Closes #1323