Skip to content

Register and delete custom containers - proto - #892

Merged
Lillian Hwang-Geddes (lvhg) merged 27 commits into
mainfrom
register-delete-custom-containers
Sep 16, 2026
Merged

Lillian Hwang-Geddes (lvhg) merged 27 commits into
mainfrom
register-delete-custom-containers

Conversation

@lvhg

Copy link
Copy Markdown
Member

string organization_id = 1;

// name format will be [user inputted name]:[first 8 characters of org id]
string name = 2;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remind me why we need :[first 8 characters of org id]?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's to prevent possible collisions between custom containers and viam containers

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What kind of collision? How would such a collision come about

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The keys are name:version, so for example a collision would happen if a user had name=tf and version=2.16 since that matches one of the viam containers


// Optional. Defaults to the image tag. Registered key is "name:version".
string version = 4;
string description = 5; // will serve as display name

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know we've already touched on this in at least one meeting, but it still seems odd to be to use description as a name. Would we run into a scenario where a user added a fairly long description, not realizing it was really a name, or will we always be masking the name of the field from them (so that they'll think they're providing the display name)? Or how will that be handled

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I see what you mean, John didn't want us to change it though since description is an existing field and changing it could cause issues. I am thinking we can add help text in the CLI to clarify things

Base automatically changed from APP-17611-list-custom-containers to main September 10, 2026 15:46

message DeleteCustomTrainingContainerRequest {
string organization_id = 1;
string key = 2;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not the id?

// visibility: VISIBILITY_PUBLIC
// created_on: 2026-01-15T01:30:15.01Z
// organization_id: "xyz"
// visibility: VISIBILITY_PUBLIC

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Duplication?

message RegisterCustomTrainingContainerRequest {
string organization_id = 1;

// On registration resolve URI to container's image:tag to set as key.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this comment is necessary?

Comment on lines +204 to +205
// Docker Hub reference, tag required: docker.io/<namespace>/<image>:<tag>.
// Official images normalize to docker.io/library/<image>:<tag>.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm trying to make sense of these comments. Would it be true to say this:

// Docker Hub image reference. Must include a tag; references without one
// are rejected at validation (no implicit :latest).
//
// The registry prefix is optional on write and normalized on read:
//   <image>:<tag>              -> docker.io/library/<image>:<tag>
//   <namespace>/<image>:<tag>  -> docker.io/<namespace>/<image>:<tag>

// On registration resolve URI to container's image:tag to set as key.
// Docker Hub reference, tag required: docker.io/<namespace>/<image>:<tag>.
// Official images normalize to docker.io/library/<image>:<tag>.
string image_uri = 3;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason image_uri is 3 and description is 5?

}

message RegisterCustomTrainingContainerResponse {
Container container = 1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Returning the id might be the better convention?

}

message DeleteCustomTrainingContainerRequest {
string organization_id = 1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If deleting by id, we do not need to include organization_id

@lvhg Lillian Hwang-Geddes (lvhg) added the ready-for-protos add this when you want protos to compile on every commit label Sep 16, 2026
@lvhg Lillian Hwang-Geddes (lvhg) removed the ready-for-protos add this when you want protos to compile on every commit label Sep 16, 2026
@lvhg
Lillian Hwang-Geddes (lvhg) merged commit fa196df into main Sep 16, 2026
2 checks passed
@lvhg
Lillian Hwang-Geddes (lvhg) deleted the register-delete-custom-containers branch September 16, 2026 22:02
Katie Peters (katiepeters) added a commit to katiepeters/viam-rdk that referenced this pull request Sep 17, 2026
Published version of viamrobotics/api#892, which adds
RegisterCustomTrainingContainer and DeleteCustomTrainingContainer to
MLTrainingService.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

protos-compiled safe to test committer is a member of this org

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants