[python] handle exceptions if they are raised by children in variables pane

[isabelizimm]

Intent

Describe briefly what problem this pull request resolves, or what new feature it introduces. Include screenshots of any new or altered UI. Link to any GitHub issues that are related.

addresses #2532

Approach

Describe the approach taken and the tradeoffs involved if non-obvious; add an overview of the solution if it's complicated.

This was occurring since fsspec.filesystem("gcs").buckets() was returning an HTTPError.

If there is any Exception (in this case it was an HTTPError), pass the error as a string to be displayed in. Also, do not inspect properties, which might execute code.

QA Notes

Add additional information for QA on how to validate the change, paying special attention to the level of risk, adjacent areas that could be affected by the change, and any important contextual information not present in the linked issues.

pip install fsspec gcsfs

import fsspec

fs = fsspec.filesystem("gcs")

and then try to expand in variables pane.

[petetronic]

Note: I had to pip install gcsfs for the QA example. I tried out your branch and was able to see the child properties of fs but had a question about the error handling.

[petetronic]

Thanks, we can iterate on how to actually format the message from the backend (open to suggestions), but this change solves the root issue.

[seeM]

LGTM – although I do have a refactoring suggestion for more flexibility down the road. I would also like to see a test that fails before this PR and passes after.

[seeM]

While this does improve Positron's UX, I'd like to propose a slight refactor for more flexibility in the future.

Currently, inspectors are totally decoupled from the variables pane and all other Positron (and even non-Positron) components. They solve the general problem of providing a consistent interface over a variety types from popular Python libraries. In theory, we could even open source inspectors as its own package in the future. Apologies, I should document this information somewhere!

This change does improve Positron's UX but it also slightly reduces the flexibility of inspectors i.e. the least surprising behavior of inspector.get_child(key) where key is a property is to raise any potential errors in calculating the property, but the user will instead get the result "Unable to show value.".

Here's how we could solve this in a more general way.

The underlying problem is the get_items call here:

positron/extensions/positron-python/python_files/positron/positron_ipykernel/variables.py

Line 708 in d6e75aa

for i, (key, value) in enumerate(get_inspector(parent).get_items()):

It raises an unhandled exception, and the frontend's RPC request times out. So I'd suggest that we catch the error at that call instead of inside get_child.

We need the value of key to pass to _summarize_variable, but currently we can't get that when get_items() errors since it yields both the key and value at the same time. I suggest replacing get_items with a new get_children method, and updating _summarize_children (and any other users/tests of get_items as necessary) to:

def _summarize_children(parent: Any) -> List[Variable]:
    num_summaries = 0
    children = get_inspector(parent).get_children()
    summaries = []
    while True:
        if num_summaries > MAX_CHILDREN:
            break
        try:
            key = next(children)
            value = e
        except StopIteration:
            break
        except Exception as e:
            logger.error(e)
            value = e  # or value = "Unable to show value."
        summary = _summarize_variable(key, value)
        if summary is not None:
            summaries.append(summary)
            num_summaries += 1
    return summaries

[isabelizimm]

Ah, that makes sense to require the inspectors always return the "real" value, and let whatever summaries handle errors. (I'm not sure of a good place to write this down, maybe a Positron wiki subfolder? 😅)

I'm happy to make these changes in a follow up PR.

[seeM]

We could start by writing it in the module docstring. It'd be great if you could add a note in your followup PR, otherwise I could also do a separate PR

[isabelizimm]

I can write that up in the follow up PR!