Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Jul 18, 2023

This PR makes a bunch of edits to the AppClient and DataClient docstrings and adds a section to the example Jupyter notebook showing how to use both classes.

@ghost ghost requested review from njooma and stuqdog July 18, 2023 17:37
@ghost ghost self-requested a review as a code owner July 18, 2023 17:37
Copy link
Member

@stuqdog stuqdog left a comment

Choose a reason for hiding this comment

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

First pass, some comments/questions but generally looking good!

)


class DataServer(DataServiceBase, DataSyncServiceBase):
Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely certain I understand what this class is doing. Can you clarify?

Copy link
Author

Choose a reason for hiding this comment

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

Yes! This is another mock data service (similar to the one that had to be created in the testing ticket). This one is used for the example documentation. This server runs on a local port and pretends to be app and feeds our example data_client--the one included in the example notebook and so displayed on our docs page--fake data just to display. This is used because we can't actually authenticate and connect to app because we don't want to include a real location secret or robot uri in the docs pages or in the python repo.

Put differently, the example page writes a function that shows how to instantiate an AppClient. We don't actually call this function though, because we know it would fail since we aren't providing real location secrets or URIs. Instead, I depend on a fake mock AppClient that directly connects its DataClient to this server^ to give the reader the illusion of a data client. Any code that would 'reveal' this illusion is marked to be hidden and so won't show up on the html page, but it's still in the repo (and still runs and shouldn't cause any error at all).

That is all!

"source": [
"from viam.proto.app.data import Filter\n",
"\n",
"data = await data_client.tabular_data_by_filter(filter=Filter(robot_name=\"arduino\"))\n",
Copy link
Member

Choose a reason for hiding this comment

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

Can we flesh out this Filter some more? Would love to show how all fields are set, or at a minimum the non-obvious fields (timestamps, tags).

@ghost ghost requested a review from stuqdog July 20, 2023 16:45
Self: the AppClient.
Self: The `AppClient`.
"""
assert dial_options.credentials.type == "robot-location-secret"
Copy link
Member

Choose a reason for hiding this comment

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

This is correct right now when we only have two credential types, but I think will be wrong when we add new types (org, user, etc.). Would prefer to assert that type is not equal to robot-secret

Copy link
Member

@stuqdog stuqdog left a comment

Choose a reason for hiding this comment

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

lgtm!

@ghost ghost merged commit 16f7853 into viamrobotics:main Jul 20, 2023
@ghost ghost deleted the RSDK-3897/update-data-docs branch July 20, 2023 18:19
This pull request was closed.
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