APP-7159: Add markdown_documentation field for inline module docs #602

michaellee1019 · 2024-12-09T20:45:46Z

The field addition is straightforward. I'm thinking about reducing the payload of APIs by separating out Model and ModelMarkdown into two different messages that APIs use. For example only GetModule, GetRegistryItem, and UpdateModule have a markdown field. But other APIs don't have it, especially List APIs. Thoughts?

michaellee1019 · 2024-12-09T21:57:28Z

separating out Model and ModelMarkdown into two different messages that APIs use

Yeah I'm going to do this. Hold off on reviewing for now.

michaellee1019 · 2024-12-12T23:14:36Z

Ok this is ready for review now. I opted for an optional field on APIs for whether to include markdown or not. Updated the scope doc too. This way we have a public way to retrieve markdown documentation, and by default it is not returned in any APIs to keep response sizes low.

…ocs-api

ehhong · 2024-12-13T16:15:39Z

proto/viam/app/v1/app.proto

+  // A list of models that are available in the module
+  repeated Model models = 1;
+  // The executable to run to start the module program
+  string entrypoint = 3;


Oops thank you!

ehhong · 2024-12-13T16:16:31Z

proto/viam/app/v1/app.proto

@@ -1132,6 +1132,7 @@ message RegistryItem {

 message GetRegistryItemRequest {
  string item_id = 1;
+  optional bool include_markdown_documentation = 2;


when do we not want to include markdown documentation?

Actually for fleet we are never going to use this in the current scope. But it will be available if we decide to show markdown on the registry pages at some point. It also makes it available to external API callers if needed.I made it so that by default, markdown is not returned so that API payloads are smaller.

My plan is to return markdown in the internal builtins service (using the same technical approach as Fancy Config Tech Spec

The alternative of the bool is to make separate messages for Request messages and Response messages. I tried that in previous commits in this PR. Its really gross and has backwards compatibility issues.

ehhong · 2024-12-13T16:17:44Z

proto/viam/app/v1/app.proto

 message Model {
  // The colon-delimited-triplet of the api implemented by the model
  string api = 1;
  // The colon-delimited-triplet of the model
  string model = 2;
+  // The markdown content describing the usage of the model
+  optional string markdown_documentation = 3;


would it be annoying to make a further nested object ModelMetadata? i can imagine we might want to include more things here later, and it feels a little jammed in Model

also, what was the decision to have separate documentation for each model rather than the entire module?

I think its fine to have high level fields for now. There will be an attributes field added in the coming months but still will be 1-to-1 to a model. I think when nesting protos, it makes sense when you need a list, not much rationale otherwise.

documentation for each model rather than the entire module

These are actually two projects that will both be done. Model-level documentation is higher priority because it solves the problem of users configuring machines. Later we will support markdown at the module level so that the module detail page can be more content rich than just the description field we have now which only allows unformatted text.

Inline Model Documentation Epic (this PR): https://viam.atlassian.net/browse/APP-6907
Embed Markdown in registry items Epic (future project): https://viam.atlassian.net/browse/APP-2349

ohh i see. i didn't understand which project this was for

ehhong · 2024-12-13T16:18:08Z

proto/viam/app/v1/app.proto

+  oneof metadata {
+    ModuleUpdateMetadata module_update_metadata = 6;
+    MLModelUpdateMetadata ml_model_update_metadata = 7;
+    MLTrainingUpdateMetadata ml_training_update_metadata = 8;


why are these called __UpdateMetadata?

Explained in this scope ammendment

I made a mistake in the scope and used the same proto messages for Update and Get APIs. Some fields are not allowed to be updated so I have created different messages to separate get/update

ah makes sense, so these are the editable fields per registry item. [opt, nit] would UpdateModuleMetadata or ModuleMetadataUpdate be clearer?

Yeah I can change to UpdateModuleMetadata. Makes sense

ehhong · 2024-12-13T16:19:01Z

proto/viam/app/v1/app.proto

@@ -1280,6 +1308,7 @@ message UploadModuleFileResponse {
 message GetModuleRequest {
  // The id of the module (formatted as prefix:name where prefix is the module owner's orgid or namespace)
  string module_id = 1;
+  optional bool include_markdown_documentation = 2;


[just curious] do we have any plans to eventually deprecate GetModule?

No official plan but I do want to migrate the CLI to use the registry item APIs. Depends on effort. I was going to do it in the fancy config project. I'm going to pickup the CLI changes for this project next week and consider doing it then.

One of the blocking things was to add the ModuleUpdateMetadata message which is one reason why I included in this PR so that I have the option to do it.

As soon as the CLI is off, the UI would need to migrate. And then probably give it another ~2-3mo for people to naturally upgrade to new versions of the CLI.

ehhong

thank you for all the clarifications! looks good

ehhong · 2024-12-13T18:26:07Z

proto/viam/app/v1/app.proto

@@ -1341,6 +1370,7 @@ message Uploads {
 message ListModulesRequest {
  // The id of the organization to return private modules for.
  optional string organization_id = 1;
+  optional bool include_markdown_documentation = 2;


i don't think ListModules would ever want to return markdown, but does this have to do with this concern?

The alternative of the bool is to make separate messages for Request messages and Response messages. I tried that in previous commits in this PR. Its really gross and has backwards compatibility issues.

Yeah this runs into the same issue that I was trying to fix by making a separate Model message. In order to not have a markdown_documentation field the response would have to be changed to a different message and cause backwards incompatibility. I could just remove this bool and always return a nil markdown. But I like the consistency of having the bool on all APIs.

…ocs-api

Add markdown_documentation field for inline module docs

362deea

github-actions bot added the safe to test committer is a member of this org label Dec 9, 2024

michaellee1019 requested review from mcous, Abalaxy, ehhong and anjinai December 9, 2024 20:48

michaellee1019 changed the title ~~Add markdown_documentation field for inline module docs~~ APP-7159: Add markdown_documentation field for inline module docs Dec 9, 2024

michaellee1019 marked this pull request as draft December 9, 2024 22:01

michaellee1019 added 3 commits December 9, 2024 17:32

update to use separate update and get metadatas

a4fddbb

different approach

8e66ce5

cleanup

bde3a03

michaellee1019 marked this pull request as ready for review December 12, 2024 23:13

michaellee1019 added bug Something isn't working and removed bug Something isn't working labels Dec 12, 2024

Merge branch 'main' of github.com:michaellee1019/api into ml/inline-d…

600c741

…ocs-api

Abalaxy approved these changes Dec 13, 2024

View reviewed changes

ehhong reviewed Dec 13, 2024

View reviewed changes

ehhong approved these changes Dec 13, 2024

View reviewed changes

michaellee1019 added 3 commits December 13, 2024 13:49

correct numbering

ea790eb

Merge branch 'main' of github.com:michaellee1019/api into ml/inline-d…

46ab805

…ocs-api

change naming

2262098

michaellee1019 added the ready-for-protos add this when you want protos to compile on every commit label Dec 16, 2024

Built new protos from 2262098

e798cf5

github-actions bot added the protos-compiled label Dec 16, 2024

michaellee1019 added bug Something isn't working and removed ready-for-protos add this when you want protos to compile on every commit labels Dec 16, 2024

github-actions bot removed the protos-compiled label Dec 16, 2024

michaellee1019 merged commit b53c73b into viamrobotics:main Dec 16, 2024
6 checks passed

michaellee1019 deleted the ml/inline-docs-api branch December 16, 2024 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

APP-7159: Add markdown_documentation field for inline module docs #602

APP-7159: Add markdown_documentation field for inline module docs #602

michaellee1019 commented Dec 9, 2024 •

edited

Loading

michaellee1019 commented Dec 9, 2024

michaellee1019 commented Dec 12, 2024

ehhong Dec 13, 2024

michaellee1019 Dec 13, 2024

ehhong Dec 13, 2024

michaellee1019 Dec 13, 2024

ehhong Dec 13, 2024

ehhong Dec 13, 2024

michaellee1019 Dec 13, 2024

ehhong Dec 13, 2024

ehhong Dec 13, 2024

michaellee1019 Dec 13, 2024

ehhong Dec 13, 2024

michaellee1019 Dec 13, 2024

ehhong Dec 13, 2024

michaellee1019 Dec 13, 2024

michaellee1019 Dec 13, 2024

ehhong left a comment

ehhong Dec 13, 2024

michaellee1019 Dec 13, 2024

APP-7159: Add markdown_documentation field for inline module docs #602

APP-7159: Add markdown_documentation field for inline module docs #602

Conversation

michaellee1019 commented Dec 9, 2024 • edited Loading

michaellee1019 commented Dec 9, 2024

michaellee1019 commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehhong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaellee1019 commented Dec 9, 2024 •

edited

Loading