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

Implement an actual error handling mechanism #677

Open
JulienVig opened this issue May 27, 2024 · 3 comments
Open

Implement an actual error handling mechanism #677

JulienVig opened this issue May 27, 2024 · 3 comments

Comments

@JulienVig
Copy link
Collaborator

Currently, errors are handled individually on the direct catch. We need a proper mechanism to communicate errors between the backend and the frontend to enable good user feedback when something fails or when users are not using DISCO in the intended way.
For example, rather than parsing the error message to identify the error, we can subtype errors to make identifiable more reliably.

@JulienVig
Copy link
Collaborator Author

This issue is getting more and more relevant to provide user feedback when an error occurs during training. We need a reliable framework to pass an error that occurs in discojs or the server to the webapp and convert it to a user-friendly error message.

Currently this is done by parsing the error message which is far from ideal:

if (error.message.includes("provided in columnConfigs does not match any of the column names")) {
      // missing field is specified between two "quotes"
      const missingFields: String = error.message.split('"')[1].split('"')[0];
      toaster.error(`The input data is missing the field "${missingFields}"`);
}

An idea would be to give specific errors identifiers such as error numbers and associate each error number with a user-friendly error message. However this requires sharing the error number table between discojs, the webapp and the server and increase interdepency.

@JulienVig
Copy link
Collaborator Author

@tharvik do you have any ideas or opinions?

@tharvik
Copy link
Collaborator

tharvik commented May 27, 2024

error handling is hard.
there is no real standard, that I know of, to safely handle error in JS/TS (as in nearly all exception based languages). there is some ideas with a JS proposal to pattern match on catch that could help a bit in a far future.

exception based:
the best we have now is documenting the various functions with @throws (but provides no compile-time checks, maybe via some eslint plugins). as you wrote, we can easily subclass Error with our own types, documenting when we're throwing theses subclasses, and try...catch at callsite.

values based:
there are some libraries, such as neverthrow, that provides safer values to force users to check that returned values are safe. this could be useful in some places where failure are excepted (opening files, read CSV, use user input, ...). as there is not language support, it's more cumbersome than in ✨Rust✨ but might worth a look.

so that doesn't really answers on what we can actually do in our case. I'm proposing the following

  • function likely to fail: use neverthrow.Result to force users to check it. at least to give some context and rethrow it.
  • function unlikely to fail: let if throw, no need to document possible failures when it's because of breach of contract (out of bound index, …)

and as the code evolves, we can put more and more everthrow around, gradually making it safer. or we can drop it and go back to classic throw/try/catch if we find it cumbersome to work with.

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

No branches or pull requests

2 participants