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

Revisit uses of RPC callMethod that (inappropriately) require kernel to be idle #4641

Open
juliasilge opened this issue Sep 11, 2024 · 0 comments
Labels
area: core Issues related to Core category.
Milestone

Comments

@juliasilge
Copy link
Contributor

While working on #4606 on passing env vars from the R session, @lionel- pointed out that our uses of callMethod() in the R extension do not resolve when the kernel is busy. This may look broken to users who, for example, may have a Shiny app running while they are trying to run some command. As of today, the uses we have are:

  • 'get_locale' to get the R locale for package testing
  • 'is_installed' to check if an R package is installed
  • newly added 'get_env_vars' for passing env vars into a package development "task"
  • a few other than I am less sure are problematic in terms of timing ('setConsoleWidth', 'isPackageAttached', 'showHelpTopic', 'showVignetteTopic')

Here are details on possible different approaches to mitigate this:

Timing

Right now the callMethod() call will only resolve when the kernel becomes idle. A better way of getting these envvars would be to push them from ark to positron after each top-level command. Positron would save the current values of these variables and use them as needed when creating tasks. This way, tasks can be run at any time rather than just when R is idle. This is not necessarily a blocker for this PR since quite often R is idle during package development, but this is still a regression for the busy case. I also wonder about the user experience as the action will not have immediate feedback in some cases, which would be confusing. So it's probably worth it to implement the push approach now.

I see two ways to go about it:

  1. Push from the kernel to positron by sending a backend-to-frontend event.

  2. Push from the kernel to the LSP. Then add an LSP request that the frontend can use without having to wait.

(1) seems the hardest (surprisingly to me, and @jmcphers might see an easier approach than I can). First it depends on who should process these events. Should it be a language-agnostic part of positron or only positron-r? If the former, it's a matter of adding the new event type on the UI comm following existing cases. If the latter, I think the UI comm is not the best place for this and we might want to create a new positron-r-specific comm that would get these events, which I think has not been done before. The advantage of such a comm is that it could be developed and experimented with "in extension" without changing core parts of positron. Once done, it should be easy to create events targetting only the positron-r extension.

In both cases, the kernel will need to to compute the current value of the envvars and send them as some kind of event, and the place to do it is https://github.com/posit-dev/ark/blob/d1f72cb03be0a2c3add85c59c147067275659eb7/crates/ark/src/interface.rs#L659.

(2) is the easiest, it requires adding the envvars as part of the ConsoleInput that we already send to the LSP after each top-level command in refresh_lsp. Then we need to implement a new LSP request to get these values. You can follow what we did for statement range for instance, starting from statement_range. I'd be happy to help with this if needed, we could pair on this for instance.

Originally posted by @lionel- in #4606 (comment)

We don't think this is likely to come up for people doing R package development, but it likely will come up for people who want to create their own tasks.

For the specific application of env vars, we might consider language agnostic infrastructure, since certainly this doesn't only apply to R. From @jmcphers:

The ability to see what environment variables are currently set in the kernel is a very useful tool in a lot of contexts, so I think it is worth considering some language-agnostic supporting infrastructure around pushing/synchronizing the set to the front end as you describe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: core Issues related to Core category.
Projects
None yet
Development

No branches or pull requests

1 participant