[RSDK-7443] paginate data #598
Conversation
There was a problem hiding this comment.
Thanks for the quick updates, and for updating BinaryDataByFilter!
Mostly just manually testing questions: Were you able to test these different permutations against the app server? Thinking of looping over 'last' and confirming that it retrieves paginated data (especially with a time interval such that you can test running out of data), requesting with count_only, using the unspecified vs a different limit, using the unspecified vs a DESCENDING sort order?
src/viam/app/data_client.py
Outdated
|
|
||
| Args: | ||
| filter (viam.proto.app.data.Filter): Optional `Filter` specifying tabular data to retrieve. No `Filter` implies all tabular | ||
| data. | ||
| limit (int): The maximum number of entries to include in a page. Defaults to 50. |
There was a problem hiding this comment.
In order to match the API undefined fields, can have this default to Optional[int] = None and specify here that if it's unspecified, we'll retrieve 50? That way if the default ever changes in the server code, we don't need to change it in the SDK code as well (and can just update the comment)
There was a problem hiding this comment.
Yup! Didn't realize the server set the limit, thought we had to do it in the client.
There was a problem hiding this comment.
Great, could possibly update the comment to 'default to 50 if unspecified' to make it clear?
src/viam/app/data_client.py
Outdated
|
|
||
| Args: | ||
| filter (viam.proto.app.data.Filter): Optional `Filter` specifying tabular data to retrieve. No `Filter` implies all tabular | ||
| data. | ||
| limit (int): The maximum number of entries to include in a page. Defaults to 50. |
There was a problem hiding this comment.
Great, could possibly update the comment to 'default to 50 if unspecified' to make it clear?
| include_binary_data (bool): Boolean specifying whether to actually include the binary file data with each retrieved file. | ||
| Defaults to true (i.e., both the files' data and metadata are returned). | ||
| count_only (bool): Whether to return only the total count of entries. | ||
| include_internal_data (bool): Whether to return the internal data. Internal data is used for Viam-specific data ingestion, |
There was a problem hiding this comment.
Since we're exposing include_internal_data exposed in the SDK, would great to add the corresponding param for TabularDataByFilter for consistency
There was a problem hiding this comment.
Should it be included?? I can remove it. I saw it in the proto so I added it
There was a problem hiding this comment.
Ok added just to make things quick
| data_request.limit = limit | ||
| if sort_order: | ||
| data_request.sort_order = sort_order | ||
| if last: |
There was a problem hiding this comment.
Awesome, thanks for updating here and in BinaryDataByFilter! Similar to my PR-level question about testing, could we confirm that keeping these unspecified gives back what we expect in an SDK call?
There was a problem hiding this comment.
I haven't manually tested this code -- I don't have a robot with a ton of data. But also, this is just client code. I added tests to make sure that the client is forwarding values properly to the server, but beyond that the client is simply going to trust that the server returns back the appropriate data for the request
There was a problem hiding this comment.
Got it, so confirming that if these are unspecified, the proto field is unspecified. If so, that matches what we'd want.
There was a problem hiding this comment.
LGTMed, but I just added you into the Data/ML Dev Org so that you can test against this, just so we can be extra sure that this is following the behavior we expect before giving customer updates.
No description provided.