positron icon indicating copy to clipboard operation
positron copied to clipboard

Forward errors when performing View actions

Open dfalbel opened this issue 1 year ago • 5 comments

This PR addresses https://github.com/posit-dev/positron/issues/3653 by forwarding the error when opening the Viewer to the user (using a notification) and suggesting to restart the session. It will also forward possible errors when performing view on Connections objects.

QA Notes

It's a little complicated to reproduce the problem:

  • Start the Python console within an environment that dioesn't have pandas installed
  • install pandas with %pip install pandas
  • Try to open a pandas data frame by clicking the data icon the variables pane.

You should now see a notification showing that something failed when opening the dataset. If you are in a dev build, you can also add some code to raise an error anywhere in the registration method:

https://github.com/posit-dev/positron/blob/07060acd646249b7d3daf86a507d741218052836/extensions/positron-python/python_files/positron/positron_ipykernel/data_explorer.py#L2257

dfalbel avatar Jul 23 '24 00:07 dfalbel

If you type %view you'll get the error on the console, which is expected. The view action comes from the variable pane, so you need to click on the the data frame button in the variables pane.

I'm not sure how to add a unit test, as in normal situations pandas would be already installed, etc. Do you have any recommendation?

dfalbel avatar Jul 26 '24 16:07 dfalbel

Ah, I just realized that if I try to view the dataframe by clicking the table icon in the variables pane, it does show the error message. But it shows after about 5 seconds which doesn't feel great. It seems like the RPC error response is not being linked back as a reply to the request message?

Otherwise it would be thrown here:

https://github.com/posit-dev/positron/blob/baefe099aee0c8eb7cee90f0b11288ed686396ae/src/vs/workbench/services/languageRuntime/common/positronBaseComm.ts#L253-L262

seeM avatar Jul 26 '24 16:07 seeM

If you type %view you'll get the error on the console, which is expected.

I see. Could we also show a more helpful error message in the console too? cannot view object of type 'pandas.core.frame.DataFrame' feels a bit misleading if we internally know it has to do with a reinstall. Maybe that's for a separate issue.

I'm not sure how to add a unit test, as in normal situations pandas would be already installed, etc. Do you have any recommendation?

Good point. We should come up with a neater way to mock scenarios where certain things aren't installed. But a rough way for now might be to patch _open_data_explorer to always raise an exception.

seeM avatar Jul 26 '24 16:07 seeM

Weird, I can't reproduce the RPC timeout on my local build:

https://github.com/user-attachments/assets/799d9e9d-05f0-48ea-bd5f-b37ce56772c3

But a rough way for now might be to patch _open_data_explorer to always raise an exception.

Thanks! Ok, I'll add something like that!

I see. Could we also show a more helpful error message in the console too? cannot view object of type 'pandas.core.frame.DataFrame' feels a bit misleading if we internally know it has to do with a reinstall. Maybe that's for a separate issue.

Makes sense, I guess I can improve the error message right away

dfalbel avatar Jul 26 '24 16:07 dfalbel

Ok, I think I fixed. I had the wrong assumption that view would call register_table() and that would fail, it actually calls is_supported() which then fails. I added an extra else statement that now captures and forwards the error to the user in this situation.

Also added a unit test.

For the %view error, we could in theory make it better, but it's tricky because this is the same error that we get when calling %view x on a non supported object and we would need a good way to detect if the object is a pandas or polars data frame without having them loaded.

dfalbel avatar Jul 26 '24 19:07 dfalbel