-
Notifications
You must be signed in to change notification settings - Fork 19
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 missing functions to data_prep_toolkit_kfp lib #113
Conversation
Signed-off-by: Revital Sur <eres@il.ibm.com>
:return: Server response object containing pipleine id and other information. | ||
""" | ||
try: | ||
p = self.kfp_client.upload_pipeline(pipeline_package_path, pipeline_name, description) |
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.
Do you need to check whether pipeline with the given name already exists? And remove the old one?
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 functionality is implemented in the https://github.com/IBM/data-prep-kit/blob/dev/kfp/kfp_support_lib/src/kfp_support/workflow_support/utils/pipelines_tests_utils.py#L6 file that already exists in the library.
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.
would it be cleaner to move this functionality here? With additional flag to overwrite or fail?
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.
Sure, will try to move the implementation. Thanks
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.
@blublinsky I changed the function as you suggested. Please review again. Thanks
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Boris Lublinsky <blublinsky@ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
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 very good, last nitpick. Lets replace print with logger
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
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.
We change libraries here, should we change the relevant versions and pipelines, or it will be a separate PR.
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
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
This PR adds missing functions to workflow_utils.py file in data_prep_toolkit_kfp lib
/closes #93