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

Protocol for extending the variables pane #560

Merged
merged 35 commits into from
Oct 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
d5c6ed7
Add methods registration and dispatching
dfalbel Sep 14, 2024
63e52e3
Dispatch on methods before relying on defaults
dfalbel Sep 14, 2024
072c4dd
add more supported methods
dfalbel Sep 14, 2024
dbfadc2
Use a strum enum to make less error prone method dispatching
dfalbel Sep 14, 2024
7f973e8
Favor let Some(x) instead of match
dfalbel Sep 14, 2024
02fcd4e
use let Ok() instead
dfalbel Sep 14, 2024
302f9a5
Move into an R based impl
dfalbel Sep 16, 2024
7473e49
minor revision
t-kalinowski Sep 17, 2024
01eda82
typo
t-kalinowski Sep 17, 2024
e0b073d
Make a method instead
dfalbel Sep 17, 2024
023a954
Add some unit tests
dfalbel Sep 18, 2024
1b8f538
Add some documentation
dfalbel Sep 24, 2024
8109668
Revise 'Extending Variables Pane` doc
t-kalinowski Sep 25, 2024
b27d308
Additional note
t-kalinowski Sep 25, 2024
894c9db
fix tests
dfalbel Oct 2, 2024
b24eaac
Find namespaced method calls like `foo:::bar`
t-kalinowski Oct 2, 2024
9bc5fc3
Remove has_ark_method
dfalbel Oct 2, 2024
588b733
use `r_task`
dfalbel Oct 2, 2024
c3ee3c6
Use `if let Err` construct
dfalbel Oct 9, 2024
cffc4ea
Use `is_string` helper
dfalbel Oct 9, 2024
7997c48
USe `cls` for consistency
dfalbel Oct 9, 2024
bd26e53
Improve readability
dfalbel Oct 9, 2024
d68648d
Simplify function
dfalbel Oct 9, 2024
a69c736
Simplify note
dfalbel Oct 9, 2024
87cd6da
Use `&self` instead
dfalbel Oct 9, 2024
2cad01e
Use `RCall` instead
dfalbel Oct 9, 2024
c8a5634
Use `RArgument`
dfalbel Oct 9, 2024
ea9c749
Add note on why we store a call
dfalbel Oct 9, 2024
17ebe9a
Use different namespacing. `.ps` -> `.ark` and `ark_variables` -> `po…
dfalbel Oct 9, 2024
9eebd2b
Move `methods.rs` to it's own module one level above.
dfalbel Oct 9, 2024
1ef4d1a
Method naming
dfalbel Oct 17, 2024
c6e52bc
Typos
dfalbel Oct 17, 2024
2ee62d7
Require explicit registration
dfalbel Oct 17, 2024
ee2f34e
Rename to `.ark.register_method()`
dfalbel Oct 17, 2024
e6e2e18
Use an allow list of packages
dfalbel Oct 17, 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
26 changes: 23 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/ark/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ yaml-rust = "0.4.5"
winsafe = { version = "0.0.19", features = ["kernel"] }
struct-field-names-as-array = "0.3.0"
strum = "0.26.2"
strum_macros = "0.26.2"
futures = "0.3.30"
tracing = "0.1.40"
tracing-subscriber = { version = "0.3.18", features = ["env-filter"] }
Expand Down
1 change: 1 addition & 0 deletions crates/ark/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ pub mod json;
pub mod logger;
pub mod logger_hprof;
pub mod lsp;
pub mod methods;
pub mod modules;
pub mod modules_utils;
pub mod plots;
Expand Down
89 changes: 89 additions & 0 deletions crates/ark/src/methods.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
//
// methods.rs
//
// Copyright (C) 2024 by Posit Software, PBC
//
//

use anyhow::anyhow;
use harp::call::RArgument;
use harp::exec::RFunction;
use harp::exec::RFunctionExt;
use harp::r_null;
use harp::utils::r_is_object;
use harp::RObject;
use libr::SEXP;
use strum_macros::Display;
use strum_macros::EnumIter;
use strum_macros::EnumString;
use strum_macros::IntoStaticStr;

use crate::modules::ARK_ENVS;

#[derive(Debug, PartialEq, EnumString, EnumIter, IntoStaticStr, Display, Eq, Hash, Clone)]
pub enum ArkGenerics {
#[strum(serialize = "ark_positron_variable_display_value")]
VariableDisplayValue,

#[strum(serialize = "ark_positron_variable_display_type")]
VariableDisplayType,

#[strum(serialize = "ark_positron_variable_has_children")]
VariableHasChildren,

#[strum(serialize = "ark_positron_variable_kind")]
VariableKind,
}

impl ArkGenerics {
// Dispatches the method on `x`
// Returns
// - `None` if no method was found,
// - `Err` if method was found and errored
// - `Err` if the method result could not be coerced to `T`
// - T, if method was found and was successfully executed
pub fn try_dispatch<T>(&self, x: SEXP, args: Vec<RArgument>) -> anyhow::Result<Option<T>>
where
// Making this a generic allows us to handle the conversion to the expected output
// type within the dispatch, which is much more ergonomic.
T: TryFrom<RObject>,
<T as TryFrom<RObject>>::Error: std::fmt::Debug,
{
if !r_is_object(x) {
return Ok(None);
}

let generic: &str = self.into();
let mut call = RFunction::new("", "call_ark_method");

call.add(generic);
call.add(x);

for RArgument { name, value } in args.into_iter() {
call.param(name.as_str(), value);
}

let result = call.call_in(ARK_ENVS.positron_ns)?;

// No method for that object
if result.sexp == r_null() {
return Ok(None);
}

// Convert the result to the expected return type
match result.try_into() {
Ok(value) => Ok(Some(value)),
Err(err) => Err(anyhow!("Conversion failed: {err:?}")),
}
}

pub fn register_method(&self, class: &str, method: RObject) -> anyhow::Result<()> {
let generic_name: &str = self.into();
RFunction::new("", ".ark.register_method")
.add(RObject::try_from(generic_name)?)
.add(RObject::try_from(class)?)
.add(method)
.call_in(ARK_ENVS.positron_ns)?;
Ok(())
}
}
61 changes: 61 additions & 0 deletions crates/ark/src/modules/positron/methods.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
#
# methods.R
#
# Copyright (C) 2024 Posit Software, PBC. All rights reserved.
#
#

ark_methods_table <- new.env(parent = emptyenv())
ark_methods_table$ark_positron_variable_display_value <- new.env(parent = emptyenv())
ark_methods_table$ark_positron_variable_display_type <- new.env(parent = emptyenv())
ark_methods_table$ark_positron_variable_has_children <- new.env(parent = emptyenv())
ark_methods_table$ark_positron_variable_kind <- new.env(parent = emptyenv())
lockEnvironment(ark_methods_table, TRUE)

ark_methods_allowed_packages <- c("torch", "reticulate")

#' Register the methods with the Positron runtime
#'
#' @param generic Generic function name as a character to register
#' @param class Class name as a character
#' @param method A method to be registered. Should be a call object.
#' @export
.ark.register_method <- function(generic, class, method) {

# Check if the caller is an allowed package
if (!in_ark_tests()) {
calling_env <- .ps.env_name(topenv(parent.frame()))
if (!(calling_env %in% paste0("namespace:", ark_methods_allowed_packages))) {
stop("Only allowed packages can register methods. Called from ", calling_env)
}
}

stopifnot(
is_string(generic),
generic %in% names(ark_methods_table),
typeof(class) == "character"
)
for (cls in class) {
assign(cls, method, envir = ark_methods_table[[generic]])
}
invisible()
}

call_ark_method <- function(generic, object, ...) {
methods_table <- ark_methods_table[[generic]]

if (is.null(methods_table)) {
return(NULL)
}

for (cls in class(object)) {
if (!is.null(method <- get0(cls, envir = methods_table))) {
return(eval(
as.call(list(method, object, ...)),
envir = globalenv()
))
Comment on lines +53 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid inlining the method in the call, we could assign the function in a child of global (either as generic or generic + class) and evaluate there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it's currently implemented method is not necessarily a function. It could be a call object, a symbol or a function.

Copy link
Contributor

Choose a reason for hiding this comment

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

A symbol? To be evaluated in global? hmmm

I think we should evaluate the input in a child of base then, and bind to a mask to avoid the inlining.

Are you sure that symbolic objects are not evaluated on the Rust side at the call_in() site? Are they protected from evaluation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory, a user could do

.ark.register_method("positron_display_values", "foo", quote(my_method_name)))

and that would call my_method_name(x) in the global env when dispatched.
I'm not sure I fully understand the risks of this approach.

}
}

NULL
}
Loading
Loading