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 try_eval_with_x() POC #443

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DavisVaughan
Copy link
Contributor

@DavisVaughan DavisVaughan commented Jul 17, 2024

POC, not done, needs discussion with @lionel- about what this should look like. Part of a larger plan to stop inlining the heck out of everything when we do an R function call - see posit-dev/positron#2797

This technically addresses posit-dev/positron#4008 and is motivated by it

The problem in that issue is that when r_format(x) calls the R level harp_format(x), it inlines x into the call. This means that when harp_format() errors and the R traceback is captured, the inlined version of x is captured in the R traceback, which we then print in the Output pane.

In this case x was an absolutely massive brmsfit object with elements in it that we don't know how to format, causing an error to get thrown from harp_format(), causing it to get inlined.

There are really two things to address here:

  • First off, we actually expect harp_format() to error sometimes, so when we propagate its Result upward, do we really want the full traceback report to be provided whenever that Error is eventually logged? It feels to me like this is an "expected" error in some cases, and seeing the full traceback is noisy and misleading. I really just want to see the message, I think.

  • Secondly, I think we need to invest in some infrastructure that allows us to replicate rlang's eval_with_x() idea. i.e. create a child env, define variables in it, and evaluate in that child env. This avoids the inline problem and results in cleaner tracebacks. I'd really like for this to be hooked into RCall and RFunction somehow, but I think we need to talk about the idea some more.

This PR starts by adding try_eval_with_x(), but it feels like the wrong level of abstraction


Reprex:

Run

library(marginaleffects)
library(brms)

mod_miss <- insight::download_model("brms_miss_1")

Then go to the Variables pane and try and expand mod_miss.

Before:

Note that the R backtrace contains the entire inlined object.

Screenshot 2024-07-17 at 4 39 35 PM

After (note that we still noisily show the R thread traceback here, but at least we can tell what is going on):

Screenshot 2024-07-17 at 4 36 17 PM

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.

1 participant