-
Notifications
You must be signed in to change notification settings - Fork 41
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
Orchestration oop #55
Conversation
…es. Incorporated code out of scaffold.py, awaiting builder.py add ins
…orate model monitoring
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
I already looked over code and gave feedback prior to the PR being created but this looks good to me. I also tried running example notebooks off of this branch and successfully hit the pipeline created endpoint.
return template.render( | ||
artifact_repo_location=artifact_repo_location, | ||
artifact_repo_name=artifact_repo_name, | ||
write_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.
With this implementation being so similar to cloud build, I think we can combine the cloudbuild and github_actions deployments into a single file with generic logic to write the jinja file then each deployment type can be a class
|
||
# Build necessary folders | ||
# TODO: make this only happen for the first component? or pull into automlops.py | ||
make_dirs([ |
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 should be one of the very first things that runs when KFP is selected as the orchestration framework. Can a pipeline exist without components? If not, I think it makes sense to keep this here. Also, if other orchestration frameworks require different directory styles, make_dirs function should be moved from utils to a method or util within the KFP classes.
filepath=comp_yaml_path, | ||
text=GENERATED_LICENSE, | ||
mode='w') | ||
write_yaml_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.
What's the difference between write_file and write_yaml_file? Could there be a type added to the argument of write_file to support yaml?
Such as:
write_file(filepath=comp_yaml_path,
text=GENERATED_LICENSE,
mode='w',
type='YAML')
Closing in favor of full OOP PR |
No description provided.