-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Added README.md for Python based visualizations #1853
Added README.md for Python based visualizations #1853
Conversation
/assign @sarahmaddox |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've added some suggestions, but overall this LGTM. I'll leave it to the repo owners to comment further.
/assign @IronPan @neuromage @rileyjbauer |
/retest |
/test kubeflow-pipeline-sample-test |
45b92a7
to
23dbf18
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really great! Very thorough! Will there be a sample that uses python visualizations?
I am not sure how we can easily create a sample to showcase Python based visualizations. But, we can change how the current Table and ROC Curve visualizations are generated/displayed to utilize Python based visualizations. This would be a limited implementation as it requires the usage of the Kubeflow Table and ROC Curve visualization components, but it would showcase Python based visualizations within current samples. What are your thoughts on this approach? |
kubeflow#1951 changes how arguments are passed from the API server to the Python service. This now allows for multi-line comment support.
d13b37a
to
ef23bdc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Just some minor nits. This looks great! Thanks @ajchili
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neuromage 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 |
* Started work on Python Based Visualization documentation * Continued work on Python Based Visualization documentation * Pipelines -> Kubeflow Pipelines * Improved wording surrounding link in output-viewer.md * Removed Jupyter reference * Removed ambiguity surrounding "Kubeflow Pipelines Team" and "users" * Removed "Why Predefined Visualizations are Important" section * Fixed brand name capitalization * Fixed header name capitalization * i.e. -> that is (spelling out abbreviations) * Removed "How to create predefined visualizations" in favor of "Next Steps" kubeflow/pipelines#1853 * Added periods to the end of each list item * Continued addressing PR feedback * Fixed TODO and added additional information to pre-intro paragraph * Added details for how to enable custom visualizations * Added details about python based visualizations vs. current offering * Added images * Changed last occurrence of Arbitrary -> Custom * Addressed PR feedback * Added version info and changed customizability -> customization Version info currently leads to dead links as releases have not yet been made * Changed version of Kubeflow release * Addressed most of the PR comments from @sarahmaddox
* Bump mlserver to 0.5.0 * Change umask and set 777 for new folders * Remove comment * Add changes in go.mod * Update storage URI in MMS tests * Update expected results * Fix linter * Update storage URIs in other tests and docs * Try extending sleep * Add more debug info * Reduce sleep * uncomment lines * Disable errexit temporarily to get debug output * Add more logs to e2e script * Kick-off tests again * Try increasing memory reqs * Remove breakpoint * Bump up memory reqs to 512 * Bump MLServer image to 0.5.3 * Revert some of the debugging changes * Change memory requirements for MMS * Try to increase memory in case is OOO again * Try reducing amount of memory * Reduce memory usage on MMS test
This change is