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.

Sign up for GitHub

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

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

Merged
merged 2 commits into from
Apr 12, 2024
Merged

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
353d987
don't time out if error
isabelizimm Apr 12, 2024
7ecd662
log error but have generic output
isabelizimm Apr 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion extensions/positron-python/python_files/positron/positron_ipykernel/inspectors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,13 @@ def get_length(self) -> int:
return len([p for p in dir(self.value) if not (p.startswith("_"))])

def get_child(self, key: str) -> Any:
return getattr(self.value, key)
if isinstance(self.value, property):
pass
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can write that up in the follow up PR!

return getattr(self.value, key)
except Exception as e:
logger.warning(msg=f"{type(e).__name__}: {e}")
return "Unable to show value."

def get_items(self) -> Iterable[Tuple[str, Any]]:
for key in dir(self.value):
Expand Down
Loading