-
Notifications
You must be signed in to change notification settings - Fork 126
APP-10775: viam training-script test-local #5524
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
| // Validate that the key portion only contains safe characters | ||
| parts := strings.SplitN(arg, "=", 2) | ||
| key := parts[0] | ||
| if !isValidArgumentKey(key) { |
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.
I tried to clean the input, let me know if there's a better way to do this.
|
|
||
| // Provide additional context for platform-related errors | ||
| errMsg := err.Error() | ||
| if strings.Contains(errMsg, "platform") || strings.Contains(errMsg, "architecture") { |
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 kind of ugly, but I thought it was worth including. it's also in the command description so potentially removable.
cli/app.go
Outdated
| Flags: []cli.Flag{ | ||
| &cli.StringFlag{ | ||
| Name: trainFlagDatasetRoot, | ||
| Usage: "path to the dataset root directory (where dataset.jsonl and image files are located). This is where you ran the 'viam dataset export' command from.", |
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.
Let me know if this is too confusing.
| └── images/ | ||
| └── cat.jpg | ||
| NOTES: |
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.
Maybe this goes in documentation? I also want to add that if the containers really slow, it could be because the dataset root or training script directory has a bunch of extra files. Apparently mounting volumes can be expensive.
| cmd.Stderr = c.App.ErrWriter | ||
|
|
||
| printf(c.App.Writer, "WARNING: If this is your first time running training, "+ | ||
| "it may take a few minutes to download the container image. "+ |
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.
A few minutes is a little bit of an understatement :)
njooma
left a comment
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.
Looks good from SDK perspective, did not review implementation details
allisonschiang
left a comment
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.
LGTM
etai-shuchatowitz
left a comment
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.
LGTM!
Adds
training-script test-localCLI command for testing ML training scripts locally in Docker before submitting to cloud.setup.py,model/training.py) and dataset paths--force-linux-pathflag todataset exportcommand to force Linux-style paths indataset.jsonl- needed to run on Windows, so we have Linux paths (to use in the container)Working on manual testing.
Usage Example
Download the dataset
Clone the classification-tflite repo
Run the below command