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

Add Positron Variables Pane Methods #1692

Merged
merged 9 commits into from
Nov 14, 2024
Merged

Conversation

t-kalinowski
Copy link
Member

@t-kalinowski t-kalinowski commented Nov 11, 2024

This PR adds support for reticulate objects in the Positron Variables Pane.

With an example session:

# needed because positron leaks an internal VIRTUAL_ENV env var into the R process
Sys.unsetenv("VIRTUAL_ENV")

library(reticulate)
library(keras3)

anint <- py_eval("1", FALSE)
adict <- py_eval("{'a':1, 'b':2}", FALSE)
adict['foo'] <- adict # recursive

alist <- py_eval("['a', 1]", FALSE)
atuple <- py_eval("('a', 1)", FALSE)

input <- keras_input(c(24, 24))

output <- input |>
  layer_dense(32, "relu") |>
  layer_dense(10, "softmax")

model <- keras_model(input, output)

We now see:

Screen.Recording.2024-11-11.at.3.12.40.PM.mov

Previously, all python objects presented as:
image

This uses the ark interface added in posit-dev/ark#560 and posit-dev/ark#561

@t-kalinowski t-kalinowski requested a review from dfalbel November 11, 2024 20:17
Copy link
Member

@dfalbel dfalbel left a comment

Choose a reason for hiding this comment

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

LGTM!

get_keys_and_children <- .globals$ark_variable_get_keys_and_children
if (is.null(get_keys_and_children)) {
get_keys_and_children <- .globals$ark_variable_get_keys_and_children <- py_eval(
"lambda i: zip(*((str(key), i.get_child(key)) for key in i.get_children()))",
Copy link
Member

Choose a reason for hiding this comment

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

I think we should limit the the length of i.get_children(), either by providing an argument from ark to ark_positron_variable_get_children or hardcoded here.

Copy link
Member Author

@t-kalinowski t-kalinowski Nov 12, 2024

Choose a reason for hiding this comment

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

What do you think the limit should be? Would it be dynamic based on something? Maybe a range of which children are visible in the pane?

If we do something here, we should also include a companion PR to make sure that the Python inspectors.get_children() is in sync. https://github.com/posit-dev/positron/blob/70e96ec21df6dbb9027b1cb96749c4a461f87616/extensions/positron-python/python_files/positron/positron_ipykernel/inspectors.py#L146

Copy link
Member

Choose a reason for hiding this comment

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

I think the UI already has a limit of what is actually displayed. This is just a hard limit to avoid building the large object that is never shown anyway. I'd add something like 100. In the Python inspectors, since this is an iterable, I think it might respect whatever the UI really needs to read?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we have a standard way to indicate that not all elements are being shown? E.g., ...truncated or similar?

Copy link
Member

Choose a reason for hiding this comment

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

The python inspector does something like:

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a python class ChildrenOverflow(), and corresponding methods so that more than 100 items are now presented as an overflow:

E.g.,

x = list(range(1000))

now shows:
image

image

if(!is_positron())
return (NULL)

x <- Sys.getenv("_")
Copy link
Member Author

Choose a reason for hiding this comment

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

@dfalbel (@lionel-?) Is there a better way to get the path to the Positron installation from R? I'm not sure if this is the most reliable way to resolve the path to the inspectors.py file that ships with Positron.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lionel- We'll open a PR to Ark, exporting a function like .ps.positron_ipykernel_path, to return the path to the positron_ipykernel python module that ships with Positron.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes that would be reasonable. .ps stands for positron so .ps_ipykernel_path?

This function could query the positron-r extension for that path via UIComm RPC.

@t-kalinowski t-kalinowski requested a review from dfalbel November 13, 2024 16:44
@t-kalinowski t-kalinowski merged commit 354a7bb into main Nov 14, 2024
16 checks passed
dfalbel added a commit to posit-dev/positron that referenced this pull request Nov 15, 2024
This PR pairs with: rstudio/reticulate#1692
The reticulate PR addresses:
#3502 (comment)

It allows Reticulate to reliably obtain the path to the Positron
`inspectors.py` which implements
the variables pane methods. By calling:

```
.ps.ui.executeCommand("positron.reticulate.getIPykernelPath")
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants