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 basic preflight checks as the first component in the pipeline #21

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JudeNiroshan
Copy link
Contributor

Address #19

Comment on lines +15 to +16
if (not repo_branch) and (repo_pr is None or repo_pr <= 0 ):
raise Exception("Both taxonomy repo branch and taxonomy pull request number cannot be empty")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This gets checked when you try to create the run. It will give an error if you try to proceed without a PR or branch.
Failed to create a new run: Failed to generate the ExecutionSpec: invalid pipeline job inputs: Invalid input error: input parameter repo_pr requires type double or integer, but the parameter value is not of number value type

Can remove this check

raise Exception("Model name is missing in kfp-model-server configMap")
if not endpoint:
raise Exception("Model Server endpoint URL is missing in kfp-model-server configMap")

Copy link
Collaborator

Choose a reason for hiding this comment

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

This will fail at the first step anyways if these params are not set correctly. Do we want to add in some additional checks here that may cause failures during the training and eval steps later on as well?

@MichaelClifford
Copy link
Collaborator

Thanks @JudeNiroshan I made a couple of comments. Can you rebase this PR as well. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants