Skip to content

Conversation

stuqdog
Copy link
Member

@stuqdog stuqdog commented Nov 9, 2023

Implements streaming_data_capture_upload

sensor_contents = SensorData(
metadata=(
SensorMetadata(
time_requested=datetime_to_timestamp(data_request_times[0]) if data_request_times[0] else None,
Copy link
Member Author

Choose a reason for hiding this comment

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

(flyby) this would have caused a runtime error in cases where the (ostensibly optional) tuple was None.

SensorData(
metadata=(
SensorMetadata(
time_requested=datetime_to_timestamp(data_request_times[idx][0]) if data_request_times[idx][0] else None,
Copy link
Member Author

Choose a reason for hiding this comment

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

(flyby) this would have caused a runtime error in cases where the (ostensibly optional) list of tuples was None.

@stuqdog stuqdog marked this pull request as ready for review November 9, 2023 21:41
@stuqdog stuqdog requested a review from a team as a code owner November 9, 2023 21:41
Copy link
Member

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

Just some qs! Don't know much about this API.

time_requested=datetime_to_timestamp(data_request_times[0]) if data_request_times[0] else None,
time_received=datetime_to_timestamp(data_request_times[1]) if data_request_times[1] else None,
time_requested=datetime_to_timestamp(data_request_times[0]) if data_request_times else None,
time_received=datetime_to_timestamp(data_request_times[1]) if data_request_times else None,
Copy link
Member

Choose a reason for hiding this comment

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

What if data_request_times[0] and not data_request_times[1]? Same question below.

Copy link
Member Author

Choose a reason for hiding this comment

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

So data_request_times is annotated as an Optional[Tuple[datetime, datetime]], not a Tuple[Optional[datetime], Optional[datetime]], so there should either be a tuple with two Somes or a None. So I think the case you're describing is one that's inconsistent with how we expect input. Definitely open to discussing changing what we expect though!

Copy link
Member

Choose a reason for hiding this comment

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

Ah no I misunderstood the typing; that sounds right to me thanks for the context!

component_type (Optional[str]): Optional type of the component associated with the file (e.g., "movement_sensor").
component_name (Optional[str]): Optional name of the component associated with the file.
method_name (Optional[str]): Optional name of the method associated with the file.
method_parameters (Optional[str]): Optional dictionary of the method parameters. No longer in active use.
Copy link
Member

Choose a reason for hiding this comment

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

What does "No longer in active use" mean? Should we not even have this parameter in the SDK if it's "deprecated"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good question! The no longer in active use language is cribbed from how method_parameters is annotated elsewhere in the SDK. I don't remember the details precisely, but I remember conversations with the data team basically being "We don't use this now, but we should still allow it to be passed because it does update on the backend."

Copy link
Member

Choose a reason for hiding this comment

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

Cool; so sort just short of deprecated. Seems fine to me, just wanted to call out.

Copy link
Member

@purplenicole730 purplenicole730 left a comment

Choose a reason for hiding this comment

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

LGTM! I'm not too familiar with this function, so it took me a while to get through. Nice tests as well!

@stuqdog stuqdog merged commit 6d726cf into viamrobotics:main Nov 13, 2023
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.

3 participants