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

Add Documentation for new Python Based Visualizations (v0.7) #1076

Merged

Conversation

ajchili
Copy link
Member

@ajchili ajchili commented Aug 14, 2019

This PR aims to add detailed documentation for the new Python Based Visualizations feature within KFP. Currently, this PR is awaiting the feature's release before exiting a WIP state. Suggestions and reviews are welcome and encouraged even though this is marked as WIP.


This change is Reviewable

@ajchili
Copy link
Member Author

ajchili commented Aug 14, 2019

/assign @sarahmaddox

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.

Thanks @ajchili! This will be a very useful doc. I've requested some updates to improve clarity. I've also suggested that some of the info is targeted at developers of the Kubeflow system rather than users, and therefore probably belongs in a GitHub README rather than on the website. Let me know what you think.

content/docs/pipelines/sdk/output-viewer.md Outdated Show resolved Hide resolved
content/docs/pipelines/sdk/python-based-visualizations.md Outdated Show resolved Hide resolved
content/docs/pipelines/sdk/python-based-visualizations.md Outdated Show resolved Hide resolved
content/docs/pipelines/sdk/python-based-visualizations.md Outdated Show resolved Hide resolved
content/docs/pipelines/sdk/python-based-visualizations.md Outdated Show resolved Hide resolved
content/docs/pipelines/sdk/python-based-visualizations.md Outdated Show resolved Hide resolved
visualizations can be found [below](/docs/pipelines/sdk/
python-based-visualizations/#how-to-create-predefined-visualizations).

## How to Create Predefined Visualizations
Copy link
Contributor

Choose a reason for hiding this comment

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

This section looks to me like info for KFP developers rather than users. If that's so, it should go in the KFP repo on GitHub rather than on the website. And then add a bullet item at the bottom of the page in a Next steps section that lets people know they can add predefined types by contributing to KFP. (We keep the developer-focused docs separate from the user docs, so that users don't think they need to build Kubeflow before they can use it.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to KFP and is a new PR awaiting review.

content/docs/pipelines/sdk/python-based-visualizations.md Outdated Show resolved Hide resolved
content/docs/pipelines/sdk/python-based-visualizations.md Outdated Show resolved Hide resolved
weight = 80
+++

This page describes python based visualizations, how to create them, and how to
Copy link
Contributor

Choose a reason for hiding this comment

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

As a reader, I wonder whether these Python-based visualisations will eventually replace the visualisations described in the other doc or whether they provide an alternative method, or whether I'm free to use both methods at once, perhaps for different reasons. It'd be good to add a bit of explanation at the top to address these questions.

Also, add info on which version of Kubeflow (e.g. v0.6 and later?) supports the new visualization method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Waiting for feature to be merged fully before adding a version it is available in.

@ajchili
Copy link
Member Author

ajchili commented Aug 15, 2019

Thanks @sarahmaddox for the detailed review! I'll make the suggested changes and push them as soon as they are finished.

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.

Thanks! Just a couple more comments from me. Let me know when you're ready for the final tech writing review. It's worth asking the dev team for a tech review at this point too.

content/docs/pipelines/sdk/python-based-visualizations.md Outdated Show resolved Hide resolved
content/docs/pipelines/sdk/python-based-visualizations.md Outdated Show resolved Hide resolved
content/docs/pipelines/sdk/python-based-visualizations.md Outdated Show resolved Hide resolved
content/docs/pipelines/sdk/python-based-visualizations.md Outdated Show resolved Hide resolved
content/docs/pipelines/sdk/python-based-visualizations.md Outdated Show resolved Hide resolved
* Added details for how to enable custom visualizations
* Added details about python based visualizations vs. current offering
@ajchili
Copy link
Member Author

ajchili commented Aug 27, 2019

/assign @neuromage

@ajchili
Copy link
Member Author

ajchili commented Aug 27, 2019

/assign @IronPan

Version info currently leads to dead links as releases have not yet been made
@ajchili ajchili changed the title [WIP] Add Documentation for new Python Based Visualizations Add Documentation for new Python Based Visualizations Aug 30, 2019
@ajchili
Copy link
Member Author

ajchili commented Aug 30, 2019

/hold
Awaiting next Kubeflow and Kubeflow Pipelines release.

@ajchili
Copy link
Member Author

ajchili commented Aug 30, 2019

/hold cancel

@sarahmaddox
Copy link
Contributor

/hold

Thanks! Since the doc now states that this feature will be available in Kubeflow v0.7 and Pipelines 0.1.28, neither of which is available yet, I've applied a hold to prevent this doc from being merged too early.

@sarahmaddox
Copy link
Contributor

@ajchili Is this doc now ready for final tech writer review, or are you awaiting a review from @IronPan first? (We can do the review, even though merging must wait until the feature is released.)

@ajchili
Copy link
Member Author

ajchili commented Sep 3, 2019

@sarahmaddox it is ready for the final tech review.

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.

Thanks for these updates! The screenshots and new wording make it very clear how the visualizations UX works. Nice. I've added some comments from a tech writing review.

content/docs/pipelines/sdk/python-based-visualizations.md Outdated Show resolved Hide resolved
content/docs/pipelines/sdk/python-based-visualizations.md Outdated Show resolved Hide resolved
6. Within the card, select the **CUSTOM** visualization type then provide a
source, and any necessary arguments (the source and argument variables are
optional for custom visualizations).
7. Provide the custom visualization code.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should include some guidelines about what the Python code looks like. Recommendation: Copy any relevant bits from the contributor README, including:

  • The requirement for Python 3.
  • How to pass variables from Pipelines to the UI.
  • Anything else that the Python developer needs to know - please consult the engineering team for these details.
  • An example of the code for a simple visualization.

Copy link
Member Author

Choose a reason for hiding this comment

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

Still need to address:

  • Anything else that the Python developer needs to know - please consult the engineering team for these details.
  • An example of the code for a simple visualization.

@sarahmaddox
Copy link
Contributor

@neuromage and @IronPan Please would you review, and provide assistance where I've commented about the Python code samples needed.

@neuromage
Copy link

Overall LGTM. Adding some examples would be great as well. @ajchili we could just add the existing predefined visualizations as samples here I think.

@sarahmaddox sarahmaddox changed the title Add Documentation for new Python Based Visualizations Add Documentation for new Python Based Visualizations (v0.7) Oct 8, 2019
@sarahmaddox
Copy link
Contributor

Fixes #1212

@sarahmaddox
Copy link
Contributor

We now have an RC for Kubeflow v0.7, which means we can publish these docs.

/hold cancel

@sarahmaddox
Copy link
Contributor

/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

@k8s-ci-robot k8s-ci-robot merged commit 5b36749 into kubeflow:master Oct 12, 2019
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