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

Print exceptions as errors #1196

Merged
merged 2 commits into from
May 22, 2024
Merged

Print exceptions as errors #1196

merged 2 commits into from
May 22, 2024

Conversation

ocallesp
Copy link
Contributor

@ocallesp ocallesp commented May 17, 2024

image

@AbhitejJohn
Copy link
Contributor

Tagging the issue here: #1192

Copy link
Contributor

@AbhitejJohn AbhitejJohn left a comment

Choose a reason for hiding this comment

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

Is the error message and details the same as what the current service puts out?

@@ -77,6 +77,8 @@ private static Assembly LoadFromBase64EncodedString(string base64EncodedAssembl
}
catch (Exception e)
{
onError(e.ToString());
onOutput(e.ToString());
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not super familiar with this code - it is weird though that we have to call both onError and onOutput for an exception?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @AbhitejJohn point. but the exception is wired on the run result, i think we are doing the wrong thing after the runner has produced the reunResult, probably in the integration part

Copy link
Member

@colombod colombod May 20, 2024

Choose a reason for hiding this comment

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

look in the file doing the integration with the wasrunner :

this is where it might loose the exception as it is now serialised across

this is where results are received and passed into the kernel:

case "wasmRunner-result":

this is where the fix proposed from @AbhitejJohn will see the onError :

runRequest.onError(wasmRunnerMessage.message);

But I think is clear the issues is in this point:

polyglotNotebooks.Logger.default.info("WasmRunner execution completed");

Copy link
Contributor Author

@ocallesp ocallesp May 20, 2024

Choose a reason for hiding this comment

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

Just to clarify, the changes on this pr is only using onOutput(e.ToString());
onError() was deleted.

Copy link
Contributor

@AbhitejJohn AbhitejJohn May 20, 2024

Choose a reason for hiding this comment

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

@ocallesp : This still seems like the wrong place to make the fix since we aren't really passing the exception details through but just printing it to the output pane as Diego mentions. Can you please take a look at the suggestion above and re-work this? At the moment, this seems like a hack.

Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify, the changes on this pr is only using onOutput(e.ToString()); onError() was deleted.

No really, the point here is to fix the bug in the factory code wiring the messages from the wasmrunner and the kernel in javascript. I think we are missing the ability to complete the commands with command failed correctly. That being the case we will need better tests to make sure all is then flowing into the runresults and events that trydotnet.js consumes and then provides to the integration code with the ui part

@ocallesp ocallesp marked this pull request as ready for review May 18, 2024 02:26
@ocallesp ocallesp force-pushed the handle-exceptions branch 3 times, most recently from 8223c4b to 7503d71 Compare May 21, 2024 16:13
@colombod
Copy link
Member

@ocalles we still need the suggestion from @AbhitejJohn , the onError call in the try catch block of the wasm runner is missing.
The onError call makes the error from the exception flow as event while the RunResult is a synchronous results that contains all outputs and errors.

Do you have a test that shows that your fix flows into the CSharpProjectWithWasmRunnerIKernel and then captures in the trudtonet.js so that customer integration can display in the output pane.

@colombod colombod merged commit e87b842 into dotnet:main May 22, 2024
4 checks passed
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.

None yet

4 participants