RSDK-4977 - add ml training apis #455
Conversation
stuqdog
force-pushed
the
add-training-api-methods
branch
from
4e018ea to
4928d1f
Compare
| def close(self): | ||
| """Close opened channels used for the various service stubs initialized.""" | ||
| if self._closed: | ||
| LOGGER.debug("AppClient is already closed.") |
| @@ -263,3 +268,65 @@ def from_dm_from_extra(extra: Optional[Dict[str, Any]]) -> bool: | |||
| return False | |||
|
|
|||
| return bool(extra.get("fromDataManagement", False)) | |||
|
|
|||
|
|
|||
| def create_filter( | |||
There was a problem hiding this comment.
Couldn't put this in ViamClient because it would create a circular dependency, didn't want to leave it in DataClient because MLTrainingClient uses it too, and creating a third class (UsesFilter or something) that the relevant clients could inherit from felt clunky to me. Happy to revisit if others have opinions.
There was a problem hiding this comment.
I think it's fine here. If you really wanted, you could create a single-use mixin, but beware the pitfalls
|
|
||
| return response.id | ||
|
|
||
| async def get_training_job(self, id: str) -> TrainingJobMetadata: |
There was a problem hiding this comment.
(note to reviewers) I think there's an argument to be made for creating our own shadow type for TrainingJobMetadata. The type ultimately breaks down into a struct of primitives, a request (which is just a a string and a filter, which we already have conversion functions for), and an enum (which is relatively straightforward). So, the type isn't terribly complicated, but it is a bit nested (i.e., it contains a request which contains a filter) and I could imagine users not loving interacting with it.
I opted for the simpler thing for now because 1) why do extra work if it's not necessary, and 2) my understanding is @njooma has a preference for using the proto types when we can reasonably get away with it. But, I'm curious to get y'all's thoughts. You can see what a manually constructed TrainingJobMetadata looks like in the test file below.
There was a problem hiding this comment.
I can see how hoisting some of the fields to top-level would be useful. If we were to make a shadow class, would filter also be automatically converted to the python-native type?
There was a problem hiding this comment.
I don't think we actually have a python-native Filter type, we just have a helper function for creating a Filter. So users would still be stuck with the proto type unless we want to now create a python-native Filter. This would be a bit of a reversal from our previous approach, but maybe worth considering given the now-expanded presence of Filters?
There was a problem hiding this comment.
In that case I think we can leave this as is -- no need to create a shadow type
|
cc @mcvella |
| @@ -263,3 +268,65 @@ def from_dm_from_extra(extra: Optional[Dict[str, Any]]) -> bool: | |||
| return False | |||
|
|
|||
| return bool(extra.get("fromDataManagement", False)) | |||
|
|
|||
|
|
|||
| def create_filter( | |||
There was a problem hiding this comment.
I think it's fine here. If you really wanted, you could create a single-use mixin, but beware the pitfalls
stuqdog
force-pushed
the
add-training-api-methods
branch
from
5532825 to
65ac46e
Compare
Adds ML Training client.
(Note to reviewers) I am currently running tests to make sure these all work in practice. I expect things to work out and didn't want to delay getting reviewers on it, but it's possible some small fixes will be pushed based on those tests.