-
Notifications
You must be signed in to change notification settings - Fork 350
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
Trap KFP namespace errors and display cause appropriately #1469
Trap KFP namespace errors and display cause appropriately #1469
Conversation
Thanks for making a pull request to Elyra! To try out this branch on binder, follow this link: |
Perhaps this would be a good time to revisit the proposal to run https://kubeflow-pipelines.readthedocs.io/en/latest/source/kfp.client.html#kfp.Client.list_experiments right here https://github.com/elyra-ai/elyra/blob/master/elyra/pipeline/processor_kfp.py#L118 to "verify" that the user-provided namespace value is valid. The |
@ptitzler I like that idea a lot. And am I correct to assume that the namespace check would be the sole function of calling |
Jap. If you tune the call to only return one row max the overhead (in the grand scheme of things) to determine whether or not the namespace value is correct should be close to zero. |
elyra/pipeline/processor_kfp.py
Outdated
self.log.error(f'Could not create experiment {experiment_name}: {ae.reason} ({ae.status})') | ||
if ae.body: | ||
error_msg = json.loads(ae.body) | ||
raise RuntimeError(f'Could not create experiment {experiment_name}: {ae.reason} ({ae.status}): ' + |
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.
is the idea here that "list experiments" will fail to retrieve experiments if the namespace is invalid? In this case, we should raise an error or provide a more specific error message to the user (e.g. Invalid namespace). I think it's a little misleading to say we could not create an experiment before we actually submit it.
Also, are we using experiments in any other place? Otherwise, I believe we should use 'jobs' which is more generic.
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.
Yeah I was pondering options on this yesterday. I kept this experiment wording from further down in the file (lines 193-194) where the error came directly from the create_experiment
function.
I agree that it's misleading now that we've changed the trapping mechanism, but I hesitated to change it to something like "invalid namespace" because list_experiments
could technically fail for other reasons since the exception is originating as a result of a GET request. I haven't encountered any other scenarios, so maybe I'm overthinking it.
Maybe something generic like "Error making request to pipeline server"? Or should I just assume namespace-only errors and go with a specific message like "Invalid namespace in runtime configuration"
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.
Neither, because the messages should express what we were trying to do: "Error validating user namespace XXX ...". The fact that we are using an experiment related API call to perform the validation is not relevant to the user. We just had to pick one that utilizes the namespace information if it is provided.
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.
Neither, because the messages should express what we were trying to do: "Error validating user namespace XXX ...". The fact that we are using an experiment related API call to perform the validation is not relevant to the user. We just had to pick one that utilizes the namespace information if it is provided.
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 is super helpful to my understanding of this, thank you! That's exactly what I was trying to say but I couldn't quite get there -- accurate error messages are an art
|
Will do. Unfortunately some error bodies come with their own punctutation and others don't, so I'll just do a quick parse to check/add punctuation if needed. Then direct users to their runtime configuration settings.
Hmm. I don't remember seeing anything different in the title recently. I like the idea though. |
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 looks good. Thank you @kiersten-stokes.
Trap KFP namespace errors and display cause appropriately avoiding double scroll bars in the error dialog box.
Resolves #1467. Now that this has been trapped separately, the double scroll bars in the error details have disappeared and only the single scrollbar remains.
Screenshots/log output
When namespace is required, but no namespace is entered in the runtime config:

Log Output
When namespace entered is incorrect:

Unfortunately, in this case, the message given as response repeats itself. I'm not sure if we want to further parse that to remove it or just leave it as is.
Log Output
Developer's Certificate of Origin 1.1