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

Forward errors when performing View actions #4122

Merged
merged 4 commits into from
Jul 29, 2024
Merged

Conversation

dfalbel
Copy link
Contributor

@dfalbel dfalbel commented Jul 23, 2024

This PR addresses #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:

@dfalbel dfalbel force-pushed the bugfix/forward-vewer-errors branch from be001b0 to baefe09 Compare July 23, 2024 21:15
Copy link
Contributor

@seeM seeM left a comment

Choose a reason for hiding this comment

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

I don't seem to be getting the desired behavior, and instead see an error in the console:

UsageError: cannot view object of type 'pandas.core.frame.DataFrame'
image

Do you see the same on your side?

Could you also please add a unit test for this change?

@dfalbel
Copy link
Contributor Author

dfalbel commented Jul 26, 2024

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?

@seeM
Copy link
Contributor

seeM commented Jul 26, 2024

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:

if (Object.keys(response).includes('error')) {
const error = response.error;
// Populate the error object with the name of the error code
// for conformity with code that expects an Error object.
error.name = `RPC Error ${response.error.code}`;
throw error;
}

@seeM
Copy link
Contributor

seeM commented Jul 26, 2024

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.

@dfalbel
Copy link
Contributor Author

dfalbel commented Jul 26, 2024

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

Screen.Recording.2024-07-26.at.13.37.49.mov

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
Copy link
Contributor Author

dfalbel commented Jul 26, 2024

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 dfalbel requested a review from seeM July 26, 2024 19:48
Copy link
Contributor

@seeM seeM left a comment

Choose a reason for hiding this comment

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

LGTM. The error message now showed immediately when I tried to click the table icon.

@dfalbel dfalbel merged commit ea24316 into main Jul 29, 2024
22 checks passed
@dfalbel dfalbel deleted the bugfix/forward-vewer-errors branch July 29, 2024 22:53
@github-actions github-actions bot locked and limited conversation to collaborators Jul 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants