More documentation on backend API methods #3758
Conversation
…zhang36/pipelines into more_info_to_experiment_api
…zhang36/pipelines into more_info_to_experiment_api
…zhang36/pipelines into more_info_to_experiment_api
…zhang36/pipelines into more_info_to_experiment_api
|
This change is |
backend/api/experiment.proto
Outdated
| @@ -114,7 +116,14 @@ message GetExperimentRequest { | |||
| } | |||
|
|
|||
| message ListExperimentsRequest { | |||
| // A page token to request the next page of results. The token is acquried | |||
| // from the nextPageToken field of the response from the previous | |||
| // ListExperiment call. | |||
There was a problem hiding this comment.
nit: Shall we also mention it is optional, when passing in an empty page token, we are fetching the first page?
Some wording suggestions:
For example, when you want to fetch page #2 of a list. Pass the token from the `nextPageToken` field of the page #1 ListExperiment response.
/cc @joeliedtke
for review too
There was a problem hiding this comment.
Note, I think you can wait for a final agreement on the wording, before changing all the similar descriptions.
There was a problem hiding this comment.
I added " or can be omitted when fetching the first page."
backend/api/experiment.proto
Outdated
| // experiments than this number, the response message will contain a valid | ||
| // value in the nextPageToken field. |
There was a problem hiding this comment.
nit: the response message will contain a nextPageToken field you can use to fetch the next page.
WDYT
backend/api/experiment.proto
Outdated
| @@ -171,13 +180,16 @@ message Experiment { | |||
| STORAGESTATE_ARCHIVED = 2; | |||
| } | |||
|
|
|||
| // Output. Specify whether this experiment is in archived or available mode. | |||
backend/api/pipeline.proto
Outdated
| // Delete a pipeline version by pipeline version ID. If the deleted pipeline | ||
| // version happens to be the default pipeline version of a certain pipeline, | ||
| // the default version of that pipeline will change to the latest pipeline | ||
| // version among all the remaining pipeline versions of that pipeline. |
There was a problem hiding this comment.
discussion: what happens when the last version is deleted?
There was a problem hiding this comment.
Then the pipeline will have no version and default version is None.
We allow pipeline having no pipeline versions under it.
And that's also why we always encourage users to create run from pipeline versions instead of from pipeline.
However, since users am familiar with the previous way of creating run by specify a pipeline, we try to be backward compatible, and use the default pipeline version if users specify pipeline when they should ideally specify a pipeline version.
|
/hold for @joeliedtke to review |
|
/assign @joeliedtke The comments will end up in our API reference doc on Kubeflow website via API doc auto-generation process. So please take a look. |
|
/assign @joeliedtke |
backend/api/experiment.proto
Outdated
| rpc GetExperiment(GetExperimentRequest) returns (Experiment) { | ||
| option (google.api.http) = { | ||
| get: "/apis/v1beta1/experiments/{id}" | ||
| }; | ||
| } | ||
|
|
||
| //Find all experiments. | ||
| // Find all experiments. Support pagination and sorting on certain fields. |
There was a problem hiding this comment.
Find -> Finds
Support pagination -> Supports pagination,
Is there a way for users to identify which fields support sorting?
There was a problem hiding this comment.
Yes, the sorting related comment is with the sorting field. E.g.,
pipelines/backend/api/pipeline.proto
Line 203 in 53d35dd
And that will be reflected to the api reference in Kubeflow website like
https://screenshot.googleplex.com/RNY9KKdkwPF
backend/api/experiment.proto
Outdated
| rpc CreateExperiment(CreateExperimentRequest) returns (Experiment) { | ||
| option (google.api.http) = { | ||
| post: "/apis/v1beta1/experiments" | ||
| body: "experiment" | ||
| }; | ||
| } | ||
|
|
||
| //Find a specific experiment by ID. | ||
| // Find a specific experiment by ID. |
backend/api/experiment.proto
Outdated
| @@ -59,43 +59,45 @@ option (grpc.gateway.protoc_gen_swagger.options.openapiv2_swagger) = { | |||
| }; | |||
|
|
|||
| service ExperimentService { | |||
| //Create a new experiment. | |||
| // Create a new experiment. | |||
backend/api/experiment.proto
Outdated
| rpc ListExperiment(ListExperimentsRequest) returns (ListExperimentsResponse) { | ||
| option (google.api.http) = { | ||
| get: "/apis/v1beta1/experiments" | ||
| }; | ||
| } | ||
|
|
||
| //Delete an experiment. | ||
| // Delete an experiment. Runs and jobs inside the experiment will stay. It is |
There was a problem hiding this comment.
Suggested revision:
Deletes an experiment without deleting the experiment's runs and jobs. To avoid unexpected behaviors, delete an experiment's runs and jobs before deleting the experiment.
backend/api/experiment.proto
Outdated
| rpc DeleteExperiment(DeleteExperimentRequest) returns (google.protobuf.Empty) { | ||
| option (google.api.http) = { | ||
| delete: "/apis/v1beta1/experiments/{id}" | ||
| }; | ||
| } | ||
|
|
||
| //Archive an experiment. | ||
| // Archive an experiment. Runs and jobs inside the experiment will be archived as well. |
There was a problem hiding this comment.
Suggested revision:
Archives an experiment and the experiment's runs and jobs.
| @@ -122,6 +126,7 @@ | |||
| }, | |||
| "/apis/v1beta1/pipeline_versions/{version_id}": { | |||
| "get": { | |||
| "summary": "Get a pipeline version by pipeline version ID.", | |||
There was a problem hiding this comment.
addressed in proto file
| @@ -150,6 +156,7 @@ | |||
| ] | |||
| }, | |||
| "delete": { | |||
| "summary": "Delete a pipeline version by pipeline version ID. If the deleted pipeline\nversion happens to be the default pipeline version of a certain pipeline,\nthe default version of that pipeline will change to the latest pipeline\nversion among all the remaining pipeline versions of that pipeline. If\nthere is no remaining pipeline version, that pipeline will have no default\nversion. Examine the run_service_api.ipynb notebook to [learn more about creating a run using a pipeline version](https://github.com/kubeflow/pipelines/blob/master/tools/benchmarks/run_service_api.ipynb)", | |||
There was a problem hiding this comment.
Suggested Revision:
Deletes a pipeline version by pipeline version ID. If the deleted pipeline version is the pipeline's default pipeline version, the default version of that pipeline changes to the pipeline's most recent remaining pipeline version. If there are no remaining pipeline versions, that pipeline will not have a default version. Examine the run_service_api.ipynb notebook to [learn more about creating a run using a pipeline version](https://github.com/kubeflow/pipelines/blob/master/tools/benchmarks/run_service_api.ipynb)
There was a problem hiding this comment.
addressed in proto file
| @@ -260,7 +272,7 @@ | |||
| ] | |||
| }, | |||
| "post": { | |||
| "summary": "Add a pipeline.", | |||
| "summary": "Create a pipeline.", | |||
There was a problem hiding this comment.
addressed in proto file
| @@ -322,7 +335,7 @@ | |||
| ] | |||
| }, | |||
| "delete": { | |||
| "summary": "Delete a pipeline.", | |||
| "summary": "Delete a pipeline. All pipeline versions under this pipeline will be deleted as well.", | |||
There was a problem hiding this comment.
Suggested revision:
Deletes a pipeline and its pipeline versions.
There was a problem hiding this comment.
addressed in proto file
| @@ -593,7 +613,8 @@ | |||
| "type": "object", | |||
| "properties": { | |||
| "pipeline_url": { | |||
| "type": "string" | |||
| "type": "string", | |||
| "description": "Online storage URL of the file containing the pipeline or pipeline version definition." | |||
There was a problem hiding this comment.
Suggested revision:
URL of the pipeline definition or the pipeline version definition.
There was a problem hiding this comment.
done. Changed it in the proto.
…pipelines into more_info_to_experiment_api
…pipelines into more_info_to_experiment_api
|
/assign @joeliedtke |
| @@ -39,7 +39,7 @@ type Client struct { | |||
| } | |||
|
|
|||
| /* | |||
| ArchiveExperiment archives an experiment runs and jobs inside the experiment will be archived as well | |||
| ArchiveExperiment archives an experiment and the experiment s runs and jobs | |||
There was a problem hiding this comment.
experiment s -> experiment's
Please update lines 42, 100, and 187 for the same issue.
There was a problem hiding this comment.
The text here in go_http_client directory is auto-generated by the swagger tools. The auto-generation is based on the .proto files, and sometimes the auto-generation will produce something not very readable even the original .proto file has the proper wording.
Moreover, the auto-generated file is in generally not edited manually, since any manual revision to this file will be overridden when the auto-generation process is taking place again.
Therefore, we usually only make sure that the text in .proto files is good for reading. And ignore the text in go_http_client folder.
| @@ -126,7 +126,7 @@ func (a *Client) DeletePipeline(params *DeletePipelineParams, authInfo runtime.C | |||
| } | |||
|
|
|||
| /* | |||
| DeletePipelineVersion deletes a pipeline version by pipeline version ID if the deleted pipeline version happens to be the default pipeline version of a certain pipeline the default version of that pipeline will change to the latest pipeline version among all the remaining pipeline versions of that pipeline if there is no remaining pipeline version that pipeline will have no default version examine the run service api ipynb notebook to learn more about creating a run using a pipeline version https github com kubeflow pipelines blob master tools benchmarks run service api ipynb | |||
| DeletePipelineVersion deletes a pipeline version by pipeline version ID if the deleted pipeline version is the default pipeline version the pipeline s default version changes to the pipeline s most recent pipeline version if there are no remaining pipeline versions the pipeline will have no default version examines the run service api ipynb notebook to learn more about creating a run using a pipeline version https github com kubeflow pipelines blob master tools benchmarks run service api ipynb | |||
There was a problem hiding this comment.
pipeline s -> pipeline's
Please update this on line 129 and 216.
There was a problem hiding this comment.
Same reason as above.
| @@ -242,7 +242,7 @@ func (a *Client) ReportRunMetrics(params *ReportRunMetricsParams, authInfo runti | |||
| } | |||
|
|
|||
| /* | |||
| RetryRun res initiate a failed or terminated run | |||
| RetryRun res initiates a failed or terminated run | |||
There was a problem hiding this comment.
res initiates -> re-initiates
There was a problem hiding this comment.
Same as above. And just for the purpose of verification, the text in .proto file is actually correct.
pipelines/backend/api/run.proto
Line 133 in c38f36c
We'll just ignore the auto-generated text by swagger tool here.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: joeliedtke The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* list experiment desc * changes should be made in proto * add comments and descriptions * comments/descriptions in run.proto * comments in job.proto and pipeline.proto * try starting a new line * newline doesnt help * add swagger gen'ed file * address comments * regenerate json and client via swagger * address comments * regenerate go_http_client and swagger from proto * two periods * re-generate
* list experiment desc * changes should be made in proto * add comments and descriptions * comments/descriptions in run.proto * comments in job.proto and pipeline.proto * try starting a new line * newline doesnt help * add swagger gen'ed file * address comments * regenerate json and client via swagger * address comments * regenerate go_http_client and swagger from proto * two periods * re-generate
Known issues: swagger generated method description in go_http_client is problematic while the auto-generated method description in the json file and later in the online api reference is fine. An example is in existing method ReportRunMetrics
pipelines/backend/api/go_http_client/run_client/run_service/run_service_client.go
Line 216 in 4c49182
But since the final online api reference is fine, we'll put up with this go_http_client method description.
Fixes #3759
/assign @Bobgy
/cc @chensun