Skip to content
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

Updated the Pipelines API reference docs #1424

Merged
merged 5 commits into from
Jan 15, 2020

Conversation

OfficePop
Copy link
Contributor

@OfficePop OfficePop commented Nov 28, 2019

Numerous updates to the swagger documentation for the Pipeline API spec. I am responsible for adding descriptions to the endpoints and filter.proto links per issue 1257 but there have also been several new endpoints added to the pipeline service tag (no descriptions for those yet, will include in a future PR).

Not sure why the version number is regressed in line 177, but that's consistent with the current swagger def in the pipelines repo.


This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Hi @OfficePop. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sarahmaddox
Copy link
Contributor

/assign
/ok-to-test

@sarahmaddox
Copy link
Contributor

/assign @IronPan

@IronPan Please would you have a look at this PR too, since you've been working with @OfficePop on the API reference.

Copy link
Contributor

@sarahmaddox sarahmaddox left a comment

Choose a reason for hiding this comment

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

Well done @OfficePop it's great to see the API docs receiving this attention! I've a few suggestions, some of which will need input from @IronPan.

</tr>
<tr>
<td><a href="#operation--apis-v1beta1-jobs--id--disable-post">POST /apis/v1beta1/jobs/{id}/disable</a></td>
<td></td>
<td><p>Disable a job.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a bit more info here and below, about the significance of disabling or enabling a job?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is a kubeflow job analogous to a kubernetes job? I don't see anything describing them in the kubeflow documentation.

If they are the same, perhaps something like "Activate a job that performs a given series of runs."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated as follows:

Disable - Stops a job and all its associated runs. The job is not deleted.
Enable - Restarts a job that was previously stopped. All runs associated with the job will continue.

@@ -300,29 +319,54 @@ <h3 id="tag-PipelineService" class="swagger-summary-tag">Tag: PipelineService</h
</thead>
<tbody>
<tr>
<td><a href="#operation--apis-v1beta1-pipelines-get">GET /apis/v1beta1/pipelines</a></td>
<td><a href="#operation--apis-v1beta1-pipeline_versions-get">GET /apis/v1beta1/pipeline_versions</a></td>
Copy link
Contributor

Choose a reason for hiding this comment

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

Yaayyy pipeline versions! I believe this is a new feature. @IronPan do you know which version of Kubeflow will include Pipelines versions?

Copy link
Member

Choose a reason for hiding this comment

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

it ideally will be the next kf release

</tr>
<tr>
<td><a href="#operation--apis-v1beta1-pipelines--id--templates-get">GET /apis/v1beta1/pipelines/{id}/templates</a></td>
<td><p>Get a YAML template for the selected pipeline.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we give a bit of information about what a template is? Also, does this end point list all templates rather than returning a specific one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated as follows:

"Returns a single YAML template that contains the description, parameters, and metadata associated with the pipeline provided."

Tried to describe what is in it since I am not sure what you would actually use the template for. My guess might be portability for custom pipelines between different KF instances (Or other ML platforms?) but I can't verify that just now.

<div class="json-inner-schema">

</div>
</dd>
<dt data-property-name="description">
<span class="
Copy link
Contributor

Choose a reason for hiding this comment

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

s/id/ID

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, how about replacing "not empty" with "present" in both cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added for the second comment, unsure what is intended by the first.

@@ -300,29 +319,54 @@ <h3 id="tag-PipelineService" class="swagger-summary-tag">Tag: PipelineService</h
</thead>
<tbody>
<tr>
<td><a href="#operation--apis-v1beta1-pipelines-get">GET /apis/v1beta1/pipelines</a></td>
<td><a href="#operation--apis-v1beta1-pipeline_versions-get">GET /apis/v1beta1/pipeline_versions</a></td>
Copy link
Member

Choose a reason for hiding this comment

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

it ideally will be the next kf release

@sarahmaddox sarahmaddox changed the title Officepop patch 1 Updated the Pipelines API reference docs Dec 4, 2019
Re-generated API spec to account for update to API version, also included other requested changes from PR.
@sarahmaddox
Copy link
Contributor

Thanks for all your work on this @OfficePop!

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sarahmaddox

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants