feat(frontend): use notebook name as default pipeline name and handle TaskMissingError#524
feat(frontend): use notebook name as default pipeline name and handle TaskMissingError#524cordeirops wants to merge 14 commits intokubeflow:mainfrom
Conversation
|
@cordeirops I took the liberty of updating the PR title and adding the issue link to the end of the firsts comment - in that way the PR and issue are linked |
jesuino
left a comment
There was a problem hiding this comment.
Thanks for your contribution! The changes are looking good, I added some minor request changes.
I see you fixed two problems in the same PR and one of the problems seems to be missing a Github issue. I sugest you to create a PR per issue (1:1) and feel free to create issues when needed.
labextension/src/lib/RPCUtils.tsx
Outdated
| ]; | ||
| const result = await showDialog({ title, body, buttons }); | ||
| const clicked = result.button ? (result.button.label as string) : ''; | ||
| if (clicked === 'Open docs') { |
There was a problem hiding this comment.
We should avoid a hardcoded string and use a constant for this string
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Hi @jesuino, thanks for the review. I’ve made the requested changes and committed them. Regarding the two issues in the same PR, this is because in #507 there was a comment mentioning that it would be fixed by #510. For this reason, I updated the description of this PR to also reference #507. |
|
@jesuino can you took another quick look here please? |
hmtosi
left a comment
There was a problem hiding this comment.
@cordeirops thank you for your work. There are quite a lot of changes here, so I have only reviewed the ones for the TaskMissingError so far. I believe there is a simpler way to do this, similar to what we did in #496. Please take a look at that solution, and at my comments.
backend/kale/errors.py
Outdated
| @@ -0,0 +1,14 @@ | |||
| """Core domain exceptions for Kale (non-RPC). | |||
There was a problem hiding this comment.
It looks like the other error types are included in the rpc/errors.py file, without an entry in this file. I do not think we need to create a separate new errors.py file here, especially because the class only includes the pass keyword, and doesn't introduce other new functionality. Could you please delete this file?
backend/kale/rpc/nb.py
Outdated
| from kale import Compiler, NotebookProcessor | ||
| from kale.rpc.errors import RPCInternalError | ||
| from kale.rpc.errors import RPCInternalError, RPCTaskIsMissing | ||
| from kale.errors import TaskMissingError |
There was a problem hiding this comment.
The error type should remain the same, so the RPCTaskIsMissing import can be removed.
The from kale.errors import TaskMissingError line will not be needed, as this file should be deleted completely.
backend/kale/rpc/nb.py
Outdated
| return {"pipeline_package_path": os.path.relpath(package_path), | ||
| "pipeline_metadata": pipeline.config.to_dict()} | ||
| except TaskMissingError as e: | ||
| # Domain-specific exception raised by core components when no |
There was a problem hiding this comment.
Can you please clean up these comments to be more concise and descriptive of just the code? These explain a bit too much of the development process.
backend/kale/rpc/nb.py
Outdated
| "compilation: %s", msg) | ||
| raise RPCTaskIsMissing(details=msg, trans_id=request.trans_id) | ||
| except ValueError as e: | ||
| # kfp.compiler or graph_component may |
There was a problem hiding this comment.
Please clean up these comments as well
backend/kale/rpc/nb.py
Outdated
|
|
||
| return {"pipeline_package_path": os.path.relpath(package_path), | ||
| "pipeline_metadata": pipeline.config.to_dict()} | ||
| except TaskMissingError as e: |
There was a problem hiding this comment.
Please use the RPCUnhandledError error instead of the TaskMissingError
backend/kale/rpc/errors.py
Outdated
| INTERNAL_ERROR = 4 | ||
| SERVICE_UNAVAILABLE = 5 | ||
| UNHANDLED_ERROR = 6 | ||
| TASK_IS_MISSING = 7 |
There was a problem hiding this comment.
We should keep this as an UNHANDLED_ERROR, so this line of code will not be needed.
backend/kale/rpc/errors.py
Outdated
| message = "Service is Unavailable" | ||
|
|
||
|
|
||
| class RPCTaskIsMissing(_RPCError): |
There was a problem hiding this comment.
We can use the existing RPCUnhandledError, so we will not need this class
labextension/src/lib/RPCUtils.tsx
Outdated
| } | ||
| }; | ||
|
|
||
| const OPEN_DOCS_LABEL = 'Open docs'; |
There was a problem hiding this comment.
Instead of opening the docs, I think it would be better to generate a clear error message asking the user to use the kale editor to tag the pipeline components. This way the user will not have to leave the page they are on. I believe we can remove this functionality.
labextension/src/lib/RPCUtils.tsx
Outdated
| refresh: boolean = false, | ||
| refresh: boolean = false | ||
| ): Promise<void> => { | ||
| if (error && error.code === RPC_CALL_STATUS.TaskIsMissing) { |
There was a problem hiding this comment.
Please use the UnhandledError code instead of TaskIsMissing
| await showRpcError(this.error, refresh); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I believe this functionality can be simplified. There is some logic already for handling the UnhandledError messages (RPCUtils.tsx:13-58`) Instead of creating a new error type, I believe this function can be updated instead. I think it would be better to provide an error message directly in the pop-up that instructs the user on how to fix it, rather than directing them to the documentation. Please see #496 for a similar discussion.
|
@cordeirops sorry for the delay on the review of this one, we are cleaning the house. :) Can you take a look on @hmtosi's comments? |
@ederign No worries! Sure, I'll take a look at it. |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
b607237 to
b62355e
Compare
- Introduced TaskMissingError to signal when a pipeline lacks steps. - Updated the compiler to raise TaskMissingError if no steps are present. - Mapped TaskMissingError to RPCTaskIsMissing for RPC error handling. - Enhanced error handling in the notebook compilation process to provide user guidance for missing steps. - Added utility functions in the UI to display informative dialogs for missing pipeline steps. Signed-off-by: cordeirops <pedro.sbarainicordeiro@gmail.com>
Signed-off-by: cordeirops <pedro.sbarainicordeiro@gmail.com>
Signed-off-by: cordeirops <pedro.sbarainicordeiro@gmail.com>
Signed-off-by: cordeirops <pedro.sbarainicordeiro@gmail.com>
Signed-off-by: cordeirops <pedro.sbarainicordeiro@gmail.com>
Signed-off-by: cordeirops <pedro.sbarainicordeiro@gmail.com>
Signed-off-by: cordeirops <pedro.sbarainicordeiro@gmail.com>
Signed-off-by: cordeirops <pedro.sbarainicordeiro@gmail.com>
Signed-off-by: cordeirops <pedro.sbarainicordeiro@gmail.com>
Signed-off-by: Pedro Sbaraini Cordeiro <pedro.sbarainicordeiro@gmail.com>
Signed-off-by: cordeirops <pedro.sbarainicordeiro@gmail.com>
Signed-off-by: cordeirops <pedro.sbarainicordeiro@gmail.com>
Signed-off-by: cordeirops <pedro.sbarainicordeiro@gmail.com>
|
@hmtosi I’ve made the changes you requested. When the pipeline does not contain any steps, the error is now displayed as shown below:
|



feat: use notebook name as default pipeline name and handle TaskMissingError
Summary
This PR introduces improvements to pipeline name generation and error handling during notebook compilation.
Key Changes
pipeline_name, applying sanitization to comply with KFP naming rules.TaskMissingError, raised by the compiler when a pipeline contains no steps.TaskMissingErrortoRPCTaskIsMissingat the RPC layer and return a more actionabledetailsmessage to the client.7(TaskIsMissing).Fixes #510 and #507