[Refactor Flask III] Remove api.abort and other error handlers

[CodyCBakerPhD]

Assessing how much of the previous logic around api.abort or api.errorhandler is actually necessary with the new logger structure attached to the app

The following are two example logs that also printed messages to the dev console

The were both triggered by the temporary/fake NotImplementedError places in the dataset generation endpoint and triggered via the 'generate' button on the settings page

The first, (31) did not have the api.abort

2024-05-25_17-14-31.log

It also corresponds to this screenshot of the displayed error on the GUIDE

The second (32) used the possibly temporary decorator which has basically the same effect as the old try/except

2024-05-25_17-13-32.log

Its screenshot of the displayed error was

Furthermore, removing the api.errorhandler changed nothing in either case for either the displayed message, dev console, or log

Conclusion: I'd say we no longer need any api.abort or other error handling now that we have the core logger

@garrettmflynn WDYT?

[garrettmflynn]

This seems fine. The request now directly throws an error on the GUIDE in a way that keeps the Error type appended to the front of the message—though I don't mind that too much. Can always parse it out if desired

[garrettmflynn]

And this decision seems a bit simpler, where we remove any parsing from the backend and move it to the frontend

[CodyCBakerPhD]

From @garrettmflynn in #796 (comment)

Ah gotcha. There are no serious issues with changing the error handling to (a), though I'd keep an eye on the error messages to ensure they are still formatted as expected. If anything, this change should result in "harder" errors—in the sense that triggering an endpoint with the fetch API will error directly instead of requiring some JavaScript-side error detection on a "successful" message back.

See src/electron/renderer/src/stories/pages/guided-mode/options/utils.js line 90 for an example of this that is no longer triggered when the generate data endpoint is called.

And how does

nwb-guide/src/electron/renderer/src/stories/pages/settings/SettingsPage.js

Lines 183 to 186 in 80a9082

	).catch((error) => {
	this.notify(error.message, "error");
	throw error;
	});

interact with

nwb-guide/src/electron/renderer/src/stories/pages/guided-mode/options/utils.js

Line 90 in 80a9082

if (results?.message) throw new Error(`Request to ${url} failed: ${results.message}`);

?

All I know right now is that a good looking error is notified to the front end

[garrettmflynn]

From @garrettmflynn in #796 (comment)

Ah gotcha. There are no serious issues with changing the error handling to (a), though I'd keep an eye on the error messages to ensure they are still formatted as expected. If anything, this change should result in "harder" errors—in the sense that triggering an endpoint with the fetch API will error directly instead of requiring some JavaScript-side error detection on a "successful" message back.

See src/electron/renderer/src/stories/pages/guided-mode/options/utils.js line 90 for an example of this that is no longer triggered when the generate data endpoint is called.

And how does

nwb-guide/src/electron/renderer/src/stories/pages/settings/SettingsPage.js

Lines 183 to 186 in 80a9082

	).catch((error) => {
	this.notify(error.message, "error");
	throw error;
	});

interact with

nwb-guide/src/electron/renderer/src/stories/pages/guided-mode/options/utils.js

Line 90 in 80a9082

if (results?.message) throw new Error(`Request to ${url} failed: ${results.message}`);

?
All I know right now is that a good looking error is notified to the front end

The catch statement "catches" any errors happening in the asynchronous function above it. So previously I needed to manually throw the failure message returned from the endpoint—but now the fetch command itself is throwing the error.

[CodyCBakerPhD]

The catch statement "catches" any errors happening in the asynchronous function above it. So previously I needed to manually throw the failure message returned from the endpoint—but now the fetch command itself is throwing the error.

So does that make the frontend simpler? We can still catch the fetch right? So we don't need the extra line in run to manually throw the Error?

[garrettmflynn]

Yes, since the error handling is uniform now, I believe we can rely on the native error thrown across all requests.

[CodyCBakerPhD]

OK I removed all the places I could spot

Be sure to play around with this in the app to ensure it behaves as we both expect

[garrettmflynn]

It seems like the behavior has changed markedly since my last test.

Source Data Error

Is this sufficient for us? The entire error is still visible in the console and log file.

This can be triggered by specifying an incorrect path for the Phy interface.

Arbitrary Data Generation Error

Now when I raise a NotImplementedError in the dataset generation resource, the endpoint passes without triggering an error—despite being caught as a 500 error on the backend

If I re-support the manual error thrown when detecting result.message in the run function, this will pass—though the error received will be the same minimally informative Internal Server Error as the previous case.

[garrettmflynn]

Not sure where the difference between the two is. I can reinstate a robust error statement for both cases by adding an error handler to the main API for the Flask app—though this requires the old request.message check, albeit only for the data generation call...

[garrettmflynn]

Ah figured it out. These are not actually handled differently by the backend. Instead, the frontend calls are different.

For the metadata request, I'm using a raw fetch request on the SourceData page and parsing the result.message value in a custom way to show a portion of the traceback (which is not usually shown).

Sitting down for the week, I remember that I've had a bit of a gripe with the way fetch and Flask endpoints interact across all my current applications, as the native catch is rarely thrown. Instead, you have to parse the error out of a "correct" response if you'd like to handle it in a custom way. Even if you're using the default error-catching system (as seen in how the message was handled in the first comment, which I now realize did parse result.message).

So it seems like we will need to keep the frontend aspects of this parsing. And it might be a good idea to include a top-level exception handler to ensure we receive errors that can be formatted in an informative way.

Thoughts?

[garrettmflynn]

You got it. I've already pushed it in a comment for comparison.

[CodyCBakerPhD]

@garrettmflynn Does my latest push satisfy the issues? Restructured the message to match Python more closely

[garrettmflynn]

Not quite. Seems like both of the previous test errors (i.e. NotImplementedError in the data.py endpoint, as well as an incorrect Phy folder_path) don't actually have a type returned.

[CodyCBakerPhD]

Not quite. Seems like both of the previous test errors (i.e. NotImplementedError in the data.py endpoint, as well as an incorrect Phy folder_path) don't actually have a type returned.

My bad, f-string for the message was botched

Can you try again?

Frontend is just displaying the message right? And traceback gets console.log'ed?

[garrettmflynn]

Just a bit different than that. The frontend adds a small amount of formatting as shown here (after updating the error handler to pass back the type and message separately).

I removed the traceback, as that's shown by forwarding the stdout to the Electron window. Was relevant for showing a certain code snippet that was returned from one of the source data failures—but it adds an edge-case to handle for something that doesn't happen very often.

[garrettmflynn]

Looks great. Thanks for pushing on this!