Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Jun 23, 2023

This PR includes a new (unimplemented) data client that uses location-level authentication to make requests to app. The methods included will allow for data to be uploaded to and retrieved from app programmatically.

@ghost ghost requested review from njooma and stuqdog June 23, 2023 18:25
@ghost ghost self-requested a review as a code owner June 23, 2023 18:25
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!

Copy link
Member

@njooma njooma left a comment

Choose a reason for hiding this comment

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

Thoughts:

  1. Should we have an AppClient similar to how we have a RobotClient that manages the lifecycle of the connection? The call site could be something like app.data_client.tabular_data_by_filter(...)
  2. If not an AppClient class under which all additional app API clients fall under, then perhaps a superclass that defines connect and close, with a default implementations so that code doesn't have to be repeated for each client class?

@stuqdog
Copy link
Member

stuqdog commented Jun 26, 2023

Thoughts:

1. Should we have an `AppClient` similar to how we have a `RobotClient` that manages the lifecycle of the connection? The call site could be something like `app.data_client.tabular_data_by_filter(...)`

2. If not an `AppClient` class under which all additional app API clients fall under, then perhaps a superclass that defines `connect` and `close`, with a default implementations so that code doesn't have to be repeated for each client class?

Good questions! I agree that we should either create a broader AppClient class or have a superclass that all app API client classes inherit from. I'd initially had some hesitation out of concern that an AppClient class would be overbroad for someone who only wants access to data APIs, e.g., and hoped we could punt on this a bit to see how things are actually used. On reflection though I think I'm still operating in a "it's C++ and it's okay if we change things up" mentality, and the comparative stability of the python SDK encourages us to make a decision now.

I think I favor the following:

  1. create an AppClient class. Given the relatively small number of cloud API clients compared to the world of possible resources, I think it's fine to explicitly enumerate them within the class. So for now, the class can just have connect(...), close(), and initialize_data_client(...) as methods, and data_client, _channel, and _metadata as class data members.
  2. Refactor DataClient's connect() to take channel and metadata as args, update comment to clarify that it's not meant to be called on its own but should rather be called through an AppClient's methods. Rename it as well, to _connect() at a minimum but probably to _create() or something? making it an __init__(...) strikes me as a code smell given the desire to not have users call it directly, but I am no Python expert so I'll happily defer on this.
  3. remove DataClient's close() method (for now at least) to further encourage use of the AppClient as the user's point of entry. Adding it back is always safe if for some reason it turns out to be necessary.

@njooma
Copy link
Member

njooma commented Jun 26, 2023

Per IRL discussion:

  1. Create an AppClient that has DataClient as a member (likely a @property)
    • AppClient will do all the channel initialization/closing/life cycle managment
  2. DataClient will only have an __init__, that takes a channel (and metadata)
  3. Close doesn't really matter since it's a pure gRPC connection (not webRTC), but we should be good citizens and expose that function anyway

@ghost ghost requested review from stuqdog and njooma June 26, 2023 16:41
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.

A couple small requested documentation changes, but it's looking good!

@ghost ghost requested a review from stuqdog June 26, 2023 17:54
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!

Copy link
Member

@njooma njooma left a comment

Choose a reason for hiding this comment

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

nice

@ghost ghost merged commit 1a4d8ad into viamrobotics:main Jun 27, 2023
@ghost ghost deleted the RSDK-3628/data-client branch June 27, 2023 15:57
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