Modify train containers list command to include custom containers - #890
Conversation
| string framework = 3; // unset for org-registered | ||
| string description = 4; // will serve as display name | ||
| google.protobuf.Timestamp eol = 5; // unset for org-registered | ||
| // 6 to 8 are new fields, 7 is set only for org-registered containers: |
There was a problem hiding this comment.
Do you mean 6 is set only for org-registered?
Also, I'd make sure any comments in here are clear/helpful, as they end up as a part of documentation (ex. "6 to 8 are new fields" is not helpful, especially longterm)
There was a problem hiding this comment.
Good point, I've made the comments a bit more clear
Vignesh P (vpandiarajan20)
left a comment
Vignesh P (vpandiarajan20)
left a comment
There was a problem hiding this comment.
Should be good to go after this fix!
| google.protobuf.Timestamp eol = 5; // unset for org-registered | ||
| string organization_id = 6; | ||
| google.protobuf.Timestamp created_on = 7; // unset for Viam containers | ||
| String id = 8; |
There was a problem hiding this comment.
lowercase 'string'!
| string description = 4; // will serve as display name | ||
| google.protobuf.Timestamp eol = 5; // unset for org-registered | ||
| string organization_id = 6; | ||
| google.protobuf.Timestamp created_on = 7; // unset for Viam containers |
There was a problem hiding this comment.
Why? Does it matter if this is included?
There was a problem hiding this comment.
| google.protobuf.Timestamp eol = 5; | ||
| string key = 1; // org-registered keys are derived from the Docker container info | ||
| string uri = 2; | ||
| string framework = 3; // unset for org-registered |
There was a problem hiding this comment.
Will users know what org-registered means?
There was a problem hiding this comment.
org registered and custom containers are the same thing, maybe I should make that clearer in the comments?
There was a problem hiding this comment.
Maybe stick to "customer container" wording
| // uri: "us-docker.pkg.dev/vertex-ai/training/tf-gpu.2-15.py310:latest" | ||
| // description: "Tensorflow 2.15" | ||
| // eol: { seconds: 1772630400, nanos: 0 } // 2026-03-03T00:00:00Z | ||
| // org_id: "xyz" |
There was a problem hiding this comment.
Is this supposed to include all of the fields from Container?
| } | ||
|
|
||
| message ListSupportedContainersResponse { | ||
| map<string, Container> container_map = 1; |
There was a problem hiding this comment.
I know not a part of the PR, but why is this a map? The endpoint is List, which I would expect to return a list
There was a problem hiding this comment.
I'm not sure about this since I didn't write that part. I assume there's some part that converts it to a list. If I had to guess why they made it a map here, probably to make it easier to link the containers with their keys
Co-authored-by: Katie Peters <katiepeters@users.noreply.github.com>
…otics/api into APP-17611-list-custom-containers
Katie Peters (katiepeters)
left a comment
Katie Peters (katiepeters)
left a comment
There was a problem hiding this comment.
Only have the one comment - wondering why created_on does not apply to Viam (public) containers
| string description = 4; // will serve as display name | ||
| google.protobuf.Timestamp eol = 5; // unset for org-registered | ||
| string organization_id = 6; | ||
| google.protobuf.Timestamp created_on = 7; // unset for Viam containers |
There was a problem hiding this comment.
Co-authored-by: Katie Peters <katiepeters@users.noreply.github.com>
…otics/api into APP-17611-list-custom-containers
| // This response takes a full Container, but not all the fields will be dispayed by CLI: | ||
| // only name/key, uri, eol, description, framework, created_on and visibility |
There was a problem hiding this comment.
I'd remove this
Lillian Hwang-Geddes (lvhg)
added
the
ready-for-protos
Lillian Hwang-Geddes (lvhg)
removed
the
ready-for-protos
3 participants
Updates the proto messages for Container and ListSupportedContainers
https://viam.atlassian.net/browse/APP-17611