-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 code for python visualization service #1651
Conversation
…te type to be specified
/assign @IronPan |
/assign @neuromage |
Previously included to ensure python3 kernel was accessible to jupyter_client.
… dictionary order
Also adds check for missing input_path argument and returns 400 error if argument is missing.
/assign @kevinbache |
/hold |
This reverts commit a7afd7b. This was done due to issues faced by the templating engine.
Python 3.6 introduced support for f-stringsl, this results in the tests failing when run in a python 3.5 environment
/retest |
/test kubeflow-pipeline-sample-test |
* Created Exporter class * Introduced ability to specify visualization timeout (default is 100 seconds) * Added more comments * Broke up post function in VisualizationHandler to call multiple function rather than handling all logic within post function * Updated imports * Updated tests
/lgtm |
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
|
||
|
||
class Exporter: | ||
|
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.
nit: extra whitespace
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.
Did not add due to PEP8 complaining about additional whitespace.
# HTML generator and exporter object | ||
html_exporter = HTMLExporter() | ||
template_file = "templates/{}.tpl".format(template_type.value) | ||
html_exporter.template_file = str(Path.cwd() / template_file) |
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.
check out __file__
rather than cwd.
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.
That would require the usage of PurePath.parent
to determine the parent route of the file, is that ideal for this?
* Fixed missing and incorrect typings * shutdown_kernel is now private method of Exporter class
/hold |
Issue stemmed from a recreation of an exporter object per request, this was resolved by creating a global exporter.
/hold cancel |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: IronPan 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 |
This PR adds code to create a visualization service for Kubeflow Pipelines. Currently, it lacks deployment and service yaml files, these will be added with an additional PR once necessary. Additionally, due to python 3 issues encountered with Bazel, the tests are not run automatically.
This change is