Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions backend/kale/compiler.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@ def generate_dsl(self):

Returns (str): A Python executable script
"""
# Fail early if there are no steps in the pipeline.
if not hasattr(self.pipeline, "steps") or not self.pipeline.steps:
raise ValueError("Task is missing from pipeline.")

# List of lightweight components generated code
lightweight_components = [
self.generate_lightweight_component(step) for step in self.pipeline.steps
Expand Down
53 changes: 38 additions & 15 deletions backend/kale/rpc/nb.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

from kale import Compiler, NotebookProcessor, marshal
from kale.common import astutils, kfputils, kfutils, podutils
from kale.rpc.errors import RPCInternalError
from kale.rpc.errors import RPCInternalError, RPCUnhandledError
from kale.rpc.log import create_adapter

KALE_MARSHAL_DIR_POSTFIX = ".kale.marshal.dir"
Expand Down Expand Up @@ -95,20 +95,43 @@ def get_base_image(request):
# fixme: Remove the debug argument from the labextension RPC call.
def compile_notebook(request, source_notebook_path, notebook_metadata_overrides=None, debug=False):
"""Compile the notebook to KFP DSL."""
processor = NotebookProcessor(source_notebook_path, notebook_metadata_overrides)
pipeline = processor.run()
imports_and_functions = processor.get_imports_and_functions()
script_path = Compiler(pipeline, imports_and_functions).compile()
# FIXME: Why were we tapping into the Kale logger?
# instance = Kale(source_notebook_path, notebook_metadata_overrides, debug)
# instance.logger = request.log if hasattr(request, "log") else logger

package_path = kfputils.compile_pipeline(script_path, pipeline.config.pipeline_name)

return {
"pipeline_package_path": os.path.relpath(package_path),
"pipeline_metadata": pipeline.config.to_dict(),
}
try:
processor = NotebookProcessor(source_notebook_path, notebook_metadata_overrides)
pipeline = processor.run()
imports_and_functions = processor.get_imports_and_functions()
script_path = Compiler(pipeline, imports_and_functions).compile()

"""FIXME: Why were we tapping into the Kale logger?
instance = Kale(source_notebook_path,
notebook_metadata_overrides, debug)
instance.logger = request.log if
hasattr(request, "log") else logger"""

package_path = kfputils.compile_pipeline(script_path, pipeline.config.pipeline_name)

return {
"pipeline_package_path": os.path.relpath(package_path),
"pipeline_metadata": pipeline.config.to_dict(),
}
except ValueError as e:
msg = str(e)
request.log.exception("ValueError during notebook compilation: %s", msg)
if "Task is missing from pipeline" in msg:
# Provide guidance to the user about how to fix the issue.
raise RPCUnhandledError(
details=(
"The pipeline does not have any steps. "
"Please tag a cell as a pipeline step or set "
"`steps_defaults` in the notebook metadata."
),
trans_id=request.trans_id,
)
raise RPCInternalError(details=msg, trans_id=request.trans_id)
except Exception as e:
# Let the run dispatcher handle generic exceptions as unhandled,
# but log for debug purposes.
request.log.exception("Unexpected error during notebook compilation: %s", e)
raise


def validate_notebook(request, source_notebook_path, notebook_metadata_overrides=None):
Expand Down
2 changes: 1 addition & 1 deletion examples/base/candies_sharing.ipynb
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@
"name": "python",
"nbconvert_exporter": "python",
"pygments_lexer": "ipython3",
"version": "3.10.18"
"version": "3.10.19"
}
},
"nbformat": 4,
Expand Down
70 changes: 43 additions & 27 deletions labextension/src/lib/RPCUtils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import * as React from 'react';
import { NotebookPanel } from '@jupyterlab/notebook';
import { Kernel } from '@jupyterlab/services';
import NotebookUtils from './NotebookUtils';
// @ts-expect-error This module is not typed
import SanitizedHTML from 'react-sanitized-html';
import { isError, IError, IOutput } from '@jupyterlab/nbformat';
import { Notification } from '@jupyterlab/apputils';

Expand All @@ -30,11 +32,11 @@ export const globalUnhandledRejection = async (event: any) => {
// isolate the segments
const stackLines = errorStack.split('\n');
// create alert string
const alert_string = 'Unhandled Error'
const alert_string = 'Unhandled Error';
Copy link
Collaborator

Choose a reason for hiding this comment

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

The changes here on (new) lines 24-62 and 124-254 and 268-377 appear to be unrelated to the main changes of the PR. I believe the preferred method of AI-assisted coding is to accept only the bare minimum necessary changes related directly to solving the issue. The LLMs often produce extra "fixes", which can cause the overall number of changes to rapidly increase. @ederign do you feel we should keep changes like these, that are valid but not directly related to the issue?

// call the toast pop up
if (errorStack.includes('lab/extensions/')){
if (errorStack.includes('lab/extensions/')) {
// if the error is caused by a jupyterlab extension, try to isolate the extension name
const extensionName = getExtensionName(stackLines)
const extensionName = getExtensionName(stackLines);
Notification.error(`An unhandled error has been thrown.`, {
actions: [
{ label: 'Details', callback: () => NotebookUtils.showMessage(alert_string,
Expand All @@ -46,20 +48,28 @@ export const globalUnhandledRejection = async (event: any) => {
autoClose: 3000
});
} else {
const extensionName = getExtensionName(stackLines);
Notification.error(`An unhandled error has been thrown.`, {
actions: [
{ label: 'Details', callback: () => NotebookUtils.showMessage(alert_string,
["Please see console for more details."]) }
{
label: 'Details',
callback: () =>
NotebookUtils.showMessage(alert_string, [
'An unhandled error was thrown from:',
extensionName,
'Please see console for more details.'
])
}
],
autoClose: 3000
});
}
}
};

function getExtensionName(stackLines: Array<string>){
const urlSplit = stackLines.slice(1,2).toString();
const extensionSplit = urlSplit.split('@').slice(1,2).toString();
function getExtensionName(stackLines: Array<string>) {
const urlSplit = stackLines.slice(1, 2).toString();
const extensionSplit = urlSplit.split('@').slice(1, 2).toString();
const extensionParts = extensionSplit.split('/');
extensionParts.pop();
const extensionName = extensionParts.join('/');
Expand All @@ -82,7 +92,7 @@ export enum RPC_CALL_STATUS {
NotFound = 3,
InternalError = 4,
ServiceUnavailable = 5,
UnhandledError = 6,
UnhandledError = 6
}

const getRpcCodeName = (code: number) => {
Expand All @@ -104,6 +114,8 @@ const getRpcCodeName = (code: number) => {
}
};



export const rokErrorTooltip = (rokError: IRPCError) => {
return (
<React.Fragment>
Expand Down Expand Up @@ -132,12 +144,12 @@ export const executeRpc = async (
env: Kernel.IKernelConnection | NotebookPanel,
func: string,
kwargs: any = {},
ctx: { nb_path: string | null } = { nb_path: null },
ctx: { nb_path: string | null } = { nb_path: null }
) => {
const cmd: string =
'from kale.rpc.run import run as __kale_rpc_run\n' +
`__kale_rpc_result = __kale_rpc_run("${func}", '${serialize(
kwargs,
kwargs
)}', '${serialize(ctx)}')`;
console.log('Executing command: ' + cmd);
const expressions = { result: '__kale_rpc_result' };
Expand All @@ -148,7 +160,7 @@ export const executeRpc = async (
? await NotebookUtils.sendKernelRequestFromNotebook(
env,
cmd,
expressions,
expressions
)
: await NotebookUtils.sendKernelRequest(env, cmd, expressions);
} catch (e) {
Expand All @@ -158,7 +170,7 @@ export const executeRpc = async (
const error = {
rpc: `${func}`,
status: `${(e as IError).ename}: ${(e as IError).evalue}`,
output: (e as IError).traceback,
output: (e as IError).traceback
};
throw new KernelError(error);
}
Expand All @@ -174,7 +186,7 @@ export const executeRpc = async (
const error = {
rpc: `${func}`,
status: output.result.status,
output: output,
output: output
};
throw new KernelError(error);
}
Expand All @@ -193,7 +205,7 @@ export const executeRpc = async (
rpc: `${func}`,
err_message: 'Failed to parse response as JSON',
error: error,
jsonData: json_data,
jsonData: json_data
};
throw new JSONParseError(jsonError);
}
Expand All @@ -205,12 +217,11 @@ export const executeRpc = async (
err_message: parsedResult.err_message,
err_details: parsedResult.err_details,
err_cls: parsedResult.err_cls,
trans_id: parsedResult.trans_id,
trans_id: parsedResult.trans_id
};
throw new RPCError(error);
}
return parsedResult.result;

};

export const showError = async (
Expand All @@ -221,11 +232,11 @@ export const showError = async (
refresh: boolean = true,
method: string | null = null,
code: number | null = null,
trans_id: number | null = null,
trans_id: number | null = null
): Promise<void> => {
const msg: string[] = [
`Browser: ${navigator ? navigator.userAgent : 'other'}`,
`Type: ${type}`,
`Type: ${type}`
];
if (method) {
msg.push(`Method: ${method}()`);
Expand All @@ -247,8 +258,9 @@ export const showError = async (

export const showRpcError = async (
error: IRPCError,
refresh: boolean = false,
refresh: boolean = false
): Promise<void> => {

await showError(
'An RPC Error has occurred',
'RPC',
Expand All @@ -257,7 +269,7 @@ export const showRpcError = async (
refresh,
error.rpc,
error.code,
error.trans_id,
error.trans_id
);
};

Expand All @@ -267,7 +279,7 @@ export const _legacy_executeRpc = async (
kernel: Kernel.IKernelConnection,
func: string,
args: any = {},
nb_path: string | null = null,
nb_path: string | null = null
) => {
if (!nb_path && notebook) {
nb_path = notebook.context.path;
Expand Down Expand Up @@ -300,15 +312,15 @@ export const _legacy_executeRpcAndShowRPCError = async (
kernel: Kernel.IKernelConnection,
func: string,
args: any = {},
nb_path: string | null = null,
nb_path: string | null = null
) => {
try {
const result = await _legacy_executeRpc(
notebook,
kernel,
func,
args,
nb_path,
nb_path
);
return result;
} catch (error) {
Expand All @@ -321,7 +333,10 @@ export const _legacy_executeRpcAndShowRPCError = async (
};

export abstract class BaseError extends Error {
constructor(message: string, public error: any) {
constructor(
message: string,
public error: any
) {
super(message);
this.name = this.constructor.name;
this.stack = new Error(message).stack;
Expand All @@ -345,7 +360,7 @@ export class KernelError extends BaseError {
this.error.status,
JSON.stringify(this.error.output, null, 3),
refresh,
this.error.rpc,
this.error.rpc
);
}
}
Expand All @@ -363,7 +378,7 @@ export class JSONParseError extends BaseError {
this.error.error.message,
this.error.json_data,
refresh,
this.error.rpc,
this.error.rpc
);
}
}
Expand All @@ -378,3 +393,4 @@ export class RPCError extends BaseError {
await showRpcError(this.error, refresh);
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Loading