-
Notifications
You must be signed in to change notification settings - Fork 30
RSDK-7220 - Ml training wrappers #206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Content looks good. Just some minor naming changes please. Once fixed, feel free to merge without requesting another review
lib/src/app/ml_training.dart
Outdated
/// gRPC client for the [MlTrainingClient]. Used for working with ML training jobs. | ||
/// | ||
/// All calls must be authenticated. | ||
class MlTrainingClient { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ml
should be ML
: https://dart.dev/effective-dart/style#do-capitalize-acronyms-and-abbreviations-longer-than-two-letters-like-words
2 Letters should still be fully capitalized
lib/src/app/ml_training.dart
Outdated
@@ -0,0 +1,70 @@ | |||
import 'package:viam_sdk/protos/app/ml_training.dart'; | |||
|
|||
/// gRPC client for the [MlTrainingClient]. Used for working with ML training jobs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the MLTrainingClient
so this docstring is recursive lol. Should this be for the ML Training Service
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh interesting! I copied this from data.dart
but I think this is a mistake cribbed from resources. Since there's no base abstract Data
type or MLTraining
type that the clients inherit from, I think both should be copying what app.dart
does instead. Will update both here.
lib/src/app/ml_training.dart
Outdated
/// Submits a custom training job request. | ||
/// | ||
/// Returns the new job's ID. | ||
Future<String> submitCustomTrainingJob(String orgId, String datasetId, String modelName, String modelVersion, registryItemId) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] registryItemId
does not have an explicit type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
barring Naveed's comments
9c84c1b
to
b89bacc
Compare
No description provided.