Data-3395: Add GetLatestTabularData #793

gloriacai01 · 2024-11-22T15:48:24Z

We added a new Data endpoint GetLatestTabularData here, this PR adds the endpoint to python SDK.

This reverts commit d6b1d80. modified: src/viam/app/data_client.py modified: src/viam/version_metadata.py modified: tests/mocks/services.py modified: tests/test_data_client.py

njooma

minor changes around return types, otherwise looks good!

njooma · 2024-11-22T16:58:36Z

src/viam/app/data_client.py

@@ -306,6 +308,39 @@ async def tabular_data_by_mql(self, organization_id: str, mql_binary: List[bytes
        response: TabularDataByMQLResponse = await self._data_client.TabularDataByMQL(request, metadata=self._metadata)
        return [bson.decode(bson_bytes) for bson_bytes in response.raw_data]

+    async def get_latest_tabular_data(self, part_id: str, resource_name: str, resource_subtype: str, method_name: str) -> Tuple[ Optional[datetime], Optional[datetime], Dict[str, ValueTypes]]:


Since some of the return values are optional, I think they should be moved to be after the non-optional value in the tuple

njooma · 2024-11-22T16:58:59Z

src/viam/app/data_client.py

+            Tuple[Dict[str, ValueTypes], Optional[datetime], Optional[datetime]: A tuple containing the following:
+            Optional[datetime]: The time captured,
+            Optional[datetime]: The time synced,
+            Dict[str, ValueTypes]: The latest tabular data captured from the specified data source.


rearrange these return types to match the order they are returned in the tuple

src/viam/app/data_client.py

kaywux · 2024-11-22T17:18:49Z

src/viam/app/data_client.py

+        request = GetLatestTabularDataRequest(part_id=part_id, resource_name=resource_name, resource_subtype=resource_subtype, method_name=method_name)
+        response: GetLatestTabularDataResponse = await self._data_client.GetLatestTabularData(request, metadata=self._metadata)
+        if not response.payload:
+            return None, None, {}


If we get an empty GetLatestTabularDataResponse (aka data hasn't been synced yet for the data source) could we return None for the whole return tuple? I don't like how only the time_captured and time_synced fields are optional and the payload isn't, since those are associated with the payload and should only be returned when the payload is also present.

I'm not very familiar with python - so let me know if this is not super python-y!

yes makes sense, i changed the entire tuple to be optional so we can return None

purplenicole730

Mostly looks good to me, but wanted to hold off approving until after the tests pass

purplenicole730 · 2024-11-22T20:45:45Z

src/viam/app/data_client.py

@@ -306,6 +308,40 @@ async def tabular_data_by_mql(self, organization_id: str, mql_binary: List[bytes
        response: TabularDataByMQLResponse = await self._data_client.TabularDataByMQL(request, metadata=self._metadata)
        return [bson.decode(bson_bytes) for bson_bytes in response.raw_data]

+    async def get_latest_tabular_data(self, part_id: str, resource_name: str, resource_subtype: str, method_name: str) -> Optional[Tuple[datetime,datetime, Dict[str, ValueTypes]]]:


Suggested change

async def get_latest_tabular_data(self, part_id: str, resource_name: str, resource_subtype: str, method_name: str) -> Optional[Tuple[datetime,datetime, Dict[str, ValueTypes]]]:

async def get_latest_tabular_data(self, part_id: str, resource_name: str, resource_subtype: str, method_name: str) -> Optional[Tuple[datetime, datetime, Dict[str, ValueTypes]]]:

Just a simple whitespace thing

purplenicole730 · 2024-11-22T20:47:09Z

tests/mocks/services.py

+        data=dict_to_struct(self.tabular_response[0].data)
+        await stream.send_message(GetLatestTabularDataResponse(payload=data, time_captured=timestamp, time_synced=timestamp))
+
+


I suspect this is one too many new lines added

njooma · 2024-11-22T20:57:54Z

src/viam/app/data_client.py

+            method_name (str): The data capture method name.
+
+        Returns:
+            Optional[Tuple[Dict[str, ValueTypes], datetime, datetime]: A tuple which is None data hasn't been synced yet for the data source, otherwise the tuple contains the following:


Reorder this tuple to be the actual return order

kaywux · 2024-12-02T22:45:21Z

src/viam/app/data_client.py

@@ -298,6 +300,39 @@ async def tabular_data_by_mql(self, organization_id: str, mql_binary: List[bytes
        response: TabularDataByMQLResponse = await self._data_client.TabularDataByMQL(request, metadata=self._metadata)
        return [bson.decode(bson_bytes) for bson_bytes in response.raw_data]

+    async def get_latest_tabular_data(self, part_id: str, resource_name: str, resource_subtype: str, method_name: str) -> Optional[Tuple[Dict[str, ValueTypes], datetime, datetime]]:


Could you reorder the returned tuple to mirror the api message field ordering? Would be good to be consistent with order across SDKs. ie Tuple[datetime, datetime, Dict[str, ValueTypes]]. Here and below!

kaywux · 2024-12-02T22:46:41Z

src/viam/app/data_client.py

+            method_name (str): The data capture method name.
+
+        Returns:
+            Optional[Tuple[Dict[str, ValueTypes], datetime, datetime]]: A tuple which is None data hasn't been synced yet for the data source, otherwise the tuple contains the following:


Could also be that the latest datapoint fell outside the TTL window. Change to something like "A return value of None means that data hasn't been synced yet for the data source or the most recently captured data was over a year ago"

njooma

Approved % @kaywux's comments

kaywux · 2024-12-03T19:11:11Z

src/viam/app/data_client.py

+            method_name (str): The data capture method name.
+
+        Returns:
+            Optional[Tuple[Dict[str, ValueTypes], datetime, datetime]]: A return value of None means that data hasn't been synced yet for the data source


Comment still has wrong order in return tuple!

kaywux

Thanks Gloria! LGTM

gloriacai01 added 5 commits November 21, 2024 16:56

add GetLatestTabularData endpoint to python sdk

4ba4d82

add subtype

d6b1d80

Revert "add subtype"

edcb84e

This reverts commit d6b1d80. modified: src/viam/app/data_client.py modified: src/viam/version_metadata.py modified: tests/mocks/services.py modified: tests/test_data_client.py

add subtype fr

fb08219

please

2b0ebdc

gloriacai01 requested a review from kaywux November 22, 2024 15:48

gloriacai01 requested a review from a team as a code owner November 22, 2024 15:48

gloriacai01 requested review from njooma and purplenicole730 November 22, 2024 15:48

njooma requested changes Nov 22, 2024

View reviewed changes

kaywux requested changes Nov 22, 2024

View reviewed changes

make entire tuple optional

7381d18

gloriacai01 requested review from kaywux and njooma November 22, 2024 19:28

purplenicole730 reviewed Nov 22, 2024

View reviewed changes

njooma requested changes Nov 22, 2024

View reviewed changes

gloriacai01 added 3 commits December 2, 2024 13:18

Merge remote-tracking branch 'upstream/main' into data-3395

fc11aa6

update return comment

6112e35

lint

488806e

gloriacai01 requested a review from njooma December 2, 2024 18:26

gloriacai01 added 3 commits December 2, 2024 13:29

update return comment

9b57826

what is going on

989d95c

fix

9538838

kaywux requested changes Dec 2, 2024

View reviewed changes

njooma approved these changes Dec 3, 2024

View reviewed changes

pr comments

314e617

gloriacai01 requested a review from kaywux December 3, 2024 17:07

kaywux requested changes Dec 3, 2024

View reviewed changes

change order in comment

db4e448

gloriacai01 requested a review from kaywux December 3, 2024 19:59

kaywux approved these changes Dec 3, 2024

View reviewed changes

gloriacai01 merged commit 8c542ee into viamrobotics:main Dec 3, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Data-3395: Add GetLatestTabularData #793

Data-3395: Add GetLatestTabularData #793

gloriacai01 commented Nov 22, 2024

njooma left a comment

njooma Nov 22, 2024

njooma Nov 22, 2024

kaywux Nov 22, 2024

gloriacai01 Nov 22, 2024

purplenicole730 left a comment

purplenicole730 Nov 22, 2024

purplenicole730 Nov 22, 2024

njooma Nov 22, 2024

kaywux Dec 2, 2024

kaywux Dec 2, 2024 •

edited

Loading

njooma left a comment

kaywux Dec 3, 2024

kaywux left a comment

	async def get_latest_tabular_data(self, part_id: str, resource_name: str, resource_subtype: str, method_name: str) -> Optional[Tuple[datetime,datetime, Dict[str, ValueTypes]]]:
	async def get_latest_tabular_data(self, part_id: str, resource_name: str, resource_subtype: str, method_name: str) -> Optional[Tuple[datetime, datetime, Dict[str, ValueTypes]]]:

		data=dict_to_struct(self.tabular_response[0].data)
		await stream.send_message(GetLatestTabularDataResponse(payload=data, time_captured=timestamp, time_synced=timestamp))

Data-3395: Add GetLatestTabularData #793

Data-3395: Add GetLatestTabularData #793

Conversation

gloriacai01 commented Nov 22, 2024

njooma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purplenicole730 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaywux Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

njooma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaywux left a comment

Choose a reason for hiding this comment

kaywux Dec 2, 2024 •

edited

Loading