From 003c54331cb71c29cc219a2c33275f94a3bcfb5e Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Fri, 9 Aug 2024 18:18:19 -0300 Subject: [PATCH 01/16] Add rpc method to query reticulate interpreters --- crates/ark/src/modules/positron/reticulate.R | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 crates/ark/src/modules/positron/reticulate.R diff --git a/crates/ark/src/modules/positron/reticulate.R b/crates/ark/src/modules/positron/reticulate.R new file mode 100644 index 000000000..39862482d --- /dev/null +++ b/crates/ark/src/modules/positron/reticulate.R @@ -0,0 +1,6 @@ + +#' @export +.ps.rpc.reticulate_interpreter_path <- function() { + if (!.ps.is_installed("reticulate")) return("") + reticulate::py_discover_config()$python +} From 239a6af9ea2e99f96efa01665f5739a6c2d22a95 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Tue, 13 Aug 2024 07:30:02 -0300 Subject: [PATCH 02/16] Add events for reticulate --- crates/ark/src/lib.rs | 1 + crates/ark/src/modules/positron/reticulate.R | 10 ++ crates/ark/src/reticulate.rs | 117 +++++++++++++++++++ 3 files changed, 128 insertions(+) create mode 100644 crates/ark/src/reticulate.rs diff --git a/crates/ark/src/lib.rs b/crates/ark/src/lib.rs index 4654244e4..b650b45d3 100644 --- a/crates/ark/src/lib.rs +++ b/crates/ark/src/lib.rs @@ -26,6 +26,7 @@ pub mod modules_utils; pub mod plots; pub mod r_task; pub mod request; +pub mod reticulate; pub mod shell; pub mod signals; pub mod srcref; diff --git a/crates/ark/src/modules/positron/reticulate.R b/crates/ark/src/modules/positron/reticulate.R index 39862482d..b9cce0fec 100644 --- a/crates/ark/src/modules/positron/reticulate.R +++ b/crates/ark/src/modules/positron/reticulate.R @@ -4,3 +4,13 @@ if (!.ps.is_installed("reticulate")) return("") reticulate::py_discover_config()$python } + +#' @export +.ps.reticulate_open <- function(id) { + .ps.Call("ps_reticulate_open") +} + +#' @export +.ps.reticulate_focus <- function(id) { + .ps.Call("ps_reticulate_focus", id) +} diff --git a/crates/ark/src/reticulate.rs b/crates/ark/src/reticulate.rs new file mode 100644 index 000000000..b3216e991 --- /dev/null +++ b/crates/ark/src/reticulate.rs @@ -0,0 +1,117 @@ +use amalthea::comm::comm_channel::CommMsg; +use amalthea::comm::event::CommManagerEvent; +use amalthea::socket::comm::CommInitiator; +use amalthea::socket::comm::CommSocket; +use crossbeam::channel::Sender; +use harp::RObject; +use libr::R_NilValue; +use libr::SEXP; +use serde_json::json; +use stdext::spawn; +use stdext::unwrap; +use uuid::Uuid; + +use crate::interface::RMain; + +pub struct ReticulateService { + comm: CommSocket, + comm_manager_tx: Sender, +} + +impl ReticulateService { + fn start(comm_id: String, comm_manager_tx: Sender) -> anyhow::Result { + let comm = CommSocket::new( + CommInitiator::BackEnd, + comm_id.clone(), + String::from("positron.reticulate"), + ); + + let service = Self { + comm, + comm_manager_tx, + }; + + let event = CommManagerEvent::Opened(service.comm.clone(), serde_json::Value::Null); + unwrap!(service.comm_manager_tx.send(event), Err(e) => { + log::error!("Reticulate: Could not open comm."); + }); + + spawn!(format!("ark-reticulate-{}", comm_id), move || { + unwrap!(service.handle_messages(), Err(err) => { + log::error!("Connection Pane: Error while handling messages: {err:?}"); + }); + }); + + Ok(comm_id) + } + + fn handle_messages(&self) -> Result<(), anyhow::Error> { + loop { + let msg = unwrap!(self.comm.incoming_rx.recv(), Err(err) => { + log::error!("Reticulate: Error while receiving message from frontend: {err:?}"); + break; + }); + + if let CommMsg::Close = msg { + self.comm.outgoing_tx.send(CommMsg::Close).unwrap(); + break; + } + + // Forward data msgs to the frontend + if let CommMsg::Data(_) = msg { + self.comm.outgoing_tx.send(msg)?; + continue; + } + } + + // before finalizing the thread we make sure to send a close message to the front end + if let Err(err) = self.comm.outgoing_tx.send(CommMsg::Close) { + log::error!("Reticulate: Error while sending comm_close to front end: {err:?}"); + } + + Ok(()) + } +} + +// Creates a client instance reticulate can use to communicate with the front-end. +// We should aim at having at most **1** client per R session. +// Further actions that reticulate can ask the front-end can be requested through +// the comm_id that is returned by this function. +#[harp::register] +pub unsafe extern "C" fn ps_reticulate_open() -> Result { + let id = Uuid::new_v4().to_string(); + + // If RMain is not initialized, we are probably in testing mode, so we just don't start the connection + // and let the testing code manually do it + if RMain::initialized() { + let main = RMain::get(); + + unwrap! ( + ReticulateService::start(id.clone(), main.get_comm_manager_tx().clone()), + Err(err) => { + log::error!("Reticulate: Failed to start connection: {err:?}"); + return Err(err); + } + ); + } else { + log::warn!("Reticulate: RMain is not initialized. Connection will not be started."); + } + + Ok(RObject::from(id).into()) +} + +#[harp::register] +pub unsafe extern "C" fn ps_reticulate_focus(id: SEXP) -> Result { + let main = RMain::get(); + let comm_id: String = RObject::view(id).to::()?; + + main.get_comm_manager_tx().send(CommManagerEvent::Message( + comm_id, + CommMsg::Data(json!({ + "method": "focus", + "params": {} + })), + ))?; + + Ok(R_NilValue) +} From 475d2f6d3d9b93841c3c78368e17c077702ce91e Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Tue, 13 Aug 2024 09:54:14 -0300 Subject: [PATCH 03/16] Make sure thre's a single reticulate thread --- crates/ark/src/reticulate.rs | 71 ++++++++++++++++++++++++++++-------- 1 file changed, 55 insertions(+), 16 deletions(-) diff --git a/crates/ark/src/reticulate.rs b/crates/ark/src/reticulate.rs index b3216e991..a7a9d6970 100644 --- a/crates/ark/src/reticulate.rs +++ b/crates/ark/src/reticulate.rs @@ -1,9 +1,13 @@ +use std::sync::Mutex; + use amalthea::comm::comm_channel::CommMsg; use amalthea::comm::event::CommManagerEvent; use amalthea::socket::comm::CommInitiator; use amalthea::socket::comm::CommSocket; +use anyhow::anyhow; use crossbeam::channel::Sender; use harp::RObject; +use lazy_static::lazy_static; use libr::R_NilValue; use libr::SEXP; use serde_json::json; @@ -13,6 +17,10 @@ use uuid::Uuid; use crate::interface::RMain; +lazy_static! { + static ref RETICULATE_COMM_ID: Mutex> = Mutex::new(None); +} + pub struct ReticulateService { comm: CommSocket, comm_manager_tx: Sender, @@ -33,7 +41,7 @@ impl ReticulateService { let event = CommManagerEvent::Opened(service.comm.clone(), serde_json::Value::Null); unwrap!(service.comm_manager_tx.send(event), Err(e) => { - log::error!("Reticulate: Could not open comm."); + log::error!("Reticulate: Could not open comm. Error {e}"); }); spawn!(format!("ark-reticulate-{}", comm_id), move || { @@ -69,6 +77,19 @@ impl ReticulateService { log::error!("Reticulate: Error while sending comm_close to front end: {err:?}"); } + let mut comm_id_guard = unwrap!( + RETICULATE_COMM_ID.try_lock(), + Err(e) => { + return Err(anyhow!("Could not access comm_id. Error {}", e)); + } + ); + log::info!( + "Reticulate Thread closing {}", + (*comm_id_guard).clone().unwrap() + ); + + *comm_id_guard = None; + Ok(()) } } @@ -79,25 +100,43 @@ impl ReticulateService { // the comm_id that is returned by this function. #[harp::register] pub unsafe extern "C" fn ps_reticulate_open() -> Result { - let id = Uuid::new_v4().to_string(); + let main = RMain::get(); - // If RMain is not initialized, we are probably in testing mode, so we just don't start the connection - // and let the testing code manually do it - if RMain::initialized() { - let main = RMain::get(); + if !RMain::initialized() { + return Ok(R_NilValue); + } - unwrap! ( - ReticulateService::start(id.clone(), main.get_comm_manager_tx().clone()), - Err(err) => { - log::error!("Reticulate: Failed to start connection: {err:?}"); - return Err(err); - } - ); - } else { - log::warn!("Reticulate: RMain is not initialized. Connection will not be started."); + // If there's an id already registered, we just need to send the focus event + let mut comm_id_guard = unwrap!( + RETICULATE_COMM_ID.try_lock(), + Err(e) => { + return Err(anyhow!("Could not access comm_id. Error {}", e)); + } + ); + + if let Some(id) = (*comm_id_guard).clone() { + // There's a comm_id registered, we just send the focus event + main.get_comm_manager_tx().send(CommManagerEvent::Message( + id, + CommMsg::Data(json!({ + "method": "focus", + "params": {} + })), + ))?; + return Ok(R_NilValue); } - Ok(RObject::from(id).into()) + let id = Uuid::new_v4().to_string(); + *comm_id_guard = Some(id.clone()); + unwrap! ( + ReticulateService::start(id, main.get_comm_manager_tx().clone()), + Err(err) => { + log::error!("Reticulate: Failed to start connection: {err:?}"); + return Err(err); + } + ); + + Ok(R_NilValue) } #[harp::register] From e698822a7d11244c0aa56ded142699179ca955d5 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Tue, 13 Aug 2024 10:33:32 -0300 Subject: [PATCH 04/16] Remove unncessary reticulate focus command --- crates/ark/src/modules/positron/reticulate.R | 5 ----- crates/ark/src/reticulate.rs | 16 ---------------- 2 files changed, 21 deletions(-) diff --git a/crates/ark/src/modules/positron/reticulate.R b/crates/ark/src/modules/positron/reticulate.R index b9cce0fec..271bf8ffb 100644 --- a/crates/ark/src/modules/positron/reticulate.R +++ b/crates/ark/src/modules/positron/reticulate.R @@ -9,8 +9,3 @@ .ps.reticulate_open <- function(id) { .ps.Call("ps_reticulate_open") } - -#' @export -.ps.reticulate_focus <- function(id) { - .ps.Call("ps_reticulate_focus", id) -} diff --git a/crates/ark/src/reticulate.rs b/crates/ark/src/reticulate.rs index a7a9d6970..ef44d449e 100644 --- a/crates/ark/src/reticulate.rs +++ b/crates/ark/src/reticulate.rs @@ -138,19 +138,3 @@ pub unsafe extern "C" fn ps_reticulate_open() -> Result { Ok(R_NilValue) } - -#[harp::register] -pub unsafe extern "C" fn ps_reticulate_focus(id: SEXP) -> Result { - let main = RMain::get(); - let comm_id: String = RObject::view(id).to::()?; - - main.get_comm_manager_tx().send(CommManagerEvent::Message( - comm_id, - CommMsg::Data(json!({ - "method": "focus", - "params": {} - })), - ))?; - - Ok(R_NilValue) -} From 6ecf22133201f22b8c49139250f6c7ce32b225bd Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Tue, 13 Aug 2024 10:34:03 -0300 Subject: [PATCH 05/16] Cleanup --- crates/ark/src/reticulate.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/ark/src/reticulate.rs b/crates/ark/src/reticulate.rs index ef44d449e..68ebe1b08 100644 --- a/crates/ark/src/reticulate.rs +++ b/crates/ark/src/reticulate.rs @@ -6,7 +6,6 @@ use amalthea::socket::comm::CommInitiator; use amalthea::socket::comm::CommSocket; use anyhow::anyhow; use crossbeam::channel::Sender; -use harp::RObject; use lazy_static::lazy_static; use libr::R_NilValue; use libr::SEXP; From 30b22ce8ca0036e45161bf813b2e534f7aa6f6ac Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Fri, 16 Aug 2024 10:51:36 -0300 Subject: [PATCH 06/16] Remove unnecessary id --- crates/ark/src/modules/positron/reticulate.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ark/src/modules/positron/reticulate.R b/crates/ark/src/modules/positron/reticulate.R index 271bf8ffb..5544510b0 100644 --- a/crates/ark/src/modules/positron/reticulate.R +++ b/crates/ark/src/modules/positron/reticulate.R @@ -6,6 +6,6 @@ } #' @export -.ps.reticulate_open <- function(id) { +.ps.reticulate_open <- function() { .ps.Call("ps_reticulate_open") } From 8ef58991546047957b2cd9ae54b65206beb39c5c Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Fri, 16 Aug 2024 14:51:40 -0300 Subject: [PATCH 07/16] Send code to R main thread --- crates/ark/src/reticulate.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/ark/src/reticulate.rs b/crates/ark/src/reticulate.rs index 68ebe1b08..e94045695 100644 --- a/crates/ark/src/reticulate.rs +++ b/crates/ark/src/reticulate.rs @@ -6,6 +6,8 @@ use amalthea::socket::comm::CommInitiator; use amalthea::socket::comm::CommSocket; use anyhow::anyhow; use crossbeam::channel::Sender; +use harp::environment::R_ENVS; +use harp::eval::r_parse_eval0; use lazy_static::lazy_static; use libr::R_NilValue; use libr::SEXP; @@ -15,6 +17,7 @@ use stdext::unwrap; use uuid::Uuid; use crate::interface::RMain; +use crate::r_task; lazy_static! { static ref RETICULATE_COMM_ID: Mutex> = Mutex::new(None); @@ -93,6 +96,12 @@ impl ReticulateService { } } +pub unsafe extern "C" fn ps_reticulate_r_main() { + r_task(|| { + r_parse_eval0("1 + 1", R_ENVS.global).unwrap(); + }) +} + // Creates a client instance reticulate can use to communicate with the front-end. // We should aim at having at most **1** client per R session. // Further actions that reticulate can ask the front-end can be requested through From 08de770ee9e0cfc5c68de407a7dcaaefc32530a8 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Fri, 23 Aug 2024 14:28:21 -0300 Subject: [PATCH 08/16] Input code --- crates/ark/src/reticulate.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/ark/src/reticulate.rs b/crates/ark/src/reticulate.rs index e94045695..b9342575e 100644 --- a/crates/ark/src/reticulate.rs +++ b/crates/ark/src/reticulate.rs @@ -8,6 +8,7 @@ use anyhow::anyhow; use crossbeam::channel::Sender; use harp::environment::R_ENVS; use harp::eval::r_parse_eval0; +use harp::RObject; use lazy_static::lazy_static; use libr::R_NilValue; use libr::SEXP; @@ -107,13 +108,16 @@ pub unsafe extern "C" fn ps_reticulate_r_main() { // Further actions that reticulate can ask the front-end can be requested through // the comm_id that is returned by this function. #[harp::register] -pub unsafe extern "C" fn ps_reticulate_open() -> Result { +pub unsafe extern "C" fn ps_reticulate_open(input: SEXP) -> Result { let main = RMain::get(); if !RMain::initialized() { return Ok(R_NilValue); } + let input = RObject::try_from(input)?; + let input_code: String = input.try_into()?; + // If there's an id already registered, we just need to send the focus event let mut comm_id_guard = unwrap!( RETICULATE_COMM_ID.try_lock(), @@ -128,7 +132,9 @@ pub unsafe extern "C" fn ps_reticulate_open() -> Result { id, CommMsg::Data(json!({ "method": "focus", - "params": {} + "params": { + "input": input_code + } })), ))?; return Ok(R_NilValue); From 71089bf103d9f36624ba933c2e66ef6071bcaffe Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Fri, 23 Aug 2024 14:30:20 -0300 Subject: [PATCH 09/16] Allow optional --- crates/ark/src/reticulate.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ark/src/reticulate.rs b/crates/ark/src/reticulate.rs index b9342575e..4a703c789 100644 --- a/crates/ark/src/reticulate.rs +++ b/crates/ark/src/reticulate.rs @@ -116,7 +116,7 @@ pub unsafe extern "C" fn ps_reticulate_open(input: SEXP) -> Result = input.try_into()?; // If there's an id already registered, we just need to send the focus event let mut comm_id_guard = unwrap!( From 37a665dd423c2322b6b44b8921be9aeba54bf716 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Fri, 6 Sep 2024 11:40:33 -0300 Subject: [PATCH 10/16] Correctly acquire input --- crates/ark/src/modules/positron/reticulate.R | 4 ++-- crates/ark/src/reticulate.rs | 9 --------- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/crates/ark/src/modules/positron/reticulate.R b/crates/ark/src/modules/positron/reticulate.R index 5544510b0..21cde3430 100644 --- a/crates/ark/src/modules/positron/reticulate.R +++ b/crates/ark/src/modules/positron/reticulate.R @@ -6,6 +6,6 @@ } #' @export -.ps.reticulate_open <- function() { - .ps.Call("ps_reticulate_open") +.ps.reticulate_open <- function(input="") { + .ps.Call("ps_reticulate_open", input) } diff --git a/crates/ark/src/reticulate.rs b/crates/ark/src/reticulate.rs index 4a703c789..082448788 100644 --- a/crates/ark/src/reticulate.rs +++ b/crates/ark/src/reticulate.rs @@ -6,8 +6,6 @@ use amalthea::socket::comm::CommInitiator; use amalthea::socket::comm::CommSocket; use anyhow::anyhow; use crossbeam::channel::Sender; -use harp::environment::R_ENVS; -use harp::eval::r_parse_eval0; use harp::RObject; use lazy_static::lazy_static; use libr::R_NilValue; @@ -18,7 +16,6 @@ use stdext::unwrap; use uuid::Uuid; use crate::interface::RMain; -use crate::r_task; lazy_static! { static ref RETICULATE_COMM_ID: Mutex> = Mutex::new(None); @@ -97,12 +94,6 @@ impl ReticulateService { } } -pub unsafe extern "C" fn ps_reticulate_r_main() { - r_task(|| { - r_parse_eval0("1 + 1", R_ENVS.global).unwrap(); - }) -} - // Creates a client instance reticulate can use to communicate with the front-end. // We should aim at having at most **1** client per R session. // Further actions that reticulate can ask the front-end can be requested through From 607b252a21b0075d02456b3d0c96e45689da10ae Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Tue, 10 Sep 2024 14:43:34 -0300 Subject: [PATCH 11/16] Create a method to start kernels --- crates/ark/src/modules/positron/reticulate.R | 22 ++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/crates/ark/src/modules/positron/reticulate.R b/crates/ark/src/modules/positron/reticulate.R index 21cde3430..13c72a570 100644 --- a/crates/ark/src/modules/positron/reticulate.R +++ b/crates/ark/src/modules/positron/reticulate.R @@ -9,3 +9,25 @@ .ps.reticulate_open <- function(input="") { .ps.Call("ps_reticulate_open", input) } + +#' @export +.ps.rpc.reticulate_start_kernel <- function(kernelPath, connectionFile, logFile, logLevel) { + # We execute as interactive to allow reticulate to prompt for installation + # of required environments and/or Python packages. + local_options(interactive = TRUE) + tryCatch({ + reticulate:::py_run_file_on_thread( + file = kernelPath, + args = c( + "-f", connectionFile, + "--logfile", logFile, + "--loglevel", logLevel, + "--session-mode", "console" + ) + ) + # Empty string means that no error happened. + "" + }, error = function(err) { + conditionMessage(err) + }) +} From 9955037d3bf8b50324247d4a2c27e5ad2713dea0 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Thu, 19 Sep 2024 19:30:55 -0300 Subject: [PATCH 12/16] Add install reticulate method --- crates/ark/src/modules/positron/reticulate.R | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/crates/ark/src/modules/positron/reticulate.R b/crates/ark/src/modules/positron/reticulate.R index 13c72a570..47ca87719 100644 --- a/crates/ark/src/modules/positron/reticulate.R +++ b/crates/ark/src/modules/positron/reticulate.R @@ -10,6 +10,17 @@ .ps.Call("ps_reticulate_open", input) } +#' Used by the front-end to install reticulate +#' @export +.ps.rpc.install_reticulate <- function() { + tryCatch({ + utils::install.packages("reticulate") + TRUE + }, error = function(err) { + FALSE + }) +} + #' @export .ps.rpc.reticulate_start_kernel <- function(kernelPath, connectionFile, logFile, logLevel) { # We execute as interactive to allow reticulate to prompt for installation From 49c9a99b29283179a10648c70f80296ceb0668de Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Fri, 20 Sep 2024 15:08:41 -0300 Subject: [PATCH 13/16] Add more method to make checks on R environment --- crates/ark/src/modules/positron/reticulate.R | 75 +++++++++++++++++--- 1 file changed, 65 insertions(+), 10 deletions(-) diff --git a/crates/ark/src/modules/positron/reticulate.R b/crates/ark/src/modules/positron/reticulate.R index 47ca87719..910d14e71 100644 --- a/crates/ark/src/modules/positron/reticulate.R +++ b/crates/ark/src/modules/positron/reticulate.R @@ -1,16 +1,10 @@ - -#' @export -.ps.rpc.reticulate_interpreter_path <- function() { - if (!.ps.is_installed("reticulate")) return("") - reticulate::py_discover_config()$python -} - #' @export .ps.reticulate_open <- function(input="") { .ps.Call("ps_reticulate_open", input) } #' Used by the front-end to install reticulate +#' A modal asking to install is shown before calling this. #' @export .ps.rpc.install_reticulate <- function() { tryCatch({ @@ -21,11 +15,72 @@ }) } +#' Called by the front-end right before starting the reticulate session. +#' +#' At this point it should be fine to load Python if it's not loaded, and +#' check if it can be started and if necessary packages are installed. +#' @export +.ps.rpc.reticulate_check_prerequisites <- function() { + + # This should return a list with the following fields: + # python: NULL or string + # venv: NULL or string + # ipykernel: NULL or string + # error: NULL or string + + config <- tryCatch({ + reticulate::py_discover_config() + }, error = function(err) { + err + }) + + if (inherits(config, "error")) { + # py_discover_config() can fail if the user forced a Python session + # via RETICULATE_PYTHON, but this version doesn't exist. + return(list(error = conditionMessage(config))) + } + + if (is.null(config) || is.null(config$python)) { + # The front-end will offer to install Python. + return(list(python = NULL, error = NULL)) + } + + python <- config$python + venv <- config$virtualenv + + # Check that python can be loaded, if it can't we will throw + # an error. which is unrecoverable. + config <- tryCatch({ + reticulate::py_config() + }, error = function(err) { + err + }) + + if (inherits(config, "error")) { + return(list(python = python, venv = venv, error = conditionMessage(config))) + } + + # Now check ipykernel + ipykernel <- tryCatch({ + reticulate::py_module_available("ipykernel") + }, error = function(err) { + err + }) + + if (inherits(ipykernel, "error")) { + return(list(python = python, venv = venv, error = as.character(ipykernel))) + } + + list( + python = config$python, + venv = venv, + ipykernel = ipykernel, + error = NULL + ) +} + #' @export .ps.rpc.reticulate_start_kernel <- function(kernelPath, connectionFile, logFile, logLevel) { - # We execute as interactive to allow reticulate to prompt for installation - # of required environments and/or Python packages. - local_options(interactive = TRUE) tryCatch({ reticulate:::py_run_file_on_thread( file = kernelPath, From f582e914a3d75eabcba659e5f573f3ccd557d55c Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Thu, 26 Sep 2024 11:00:05 -0300 Subject: [PATCH 14/16] Apply suggestions from code review Co-authored-by: Lionel Henry --- crates/ark/src/modules/positron/reticulate.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ark/src/modules/positron/reticulate.R b/crates/ark/src/modules/positron/reticulate.R index 910d14e71..41169b8eb 100644 --- a/crates/ark/src/modules/positron/reticulate.R +++ b/crates/ark/src/modules/positron/reticulate.R @@ -49,7 +49,7 @@ venv <- config$virtualenv # Check that python can be loaded, if it can't we will throw - # an error. which is unrecoverable. + # an error, which is unrecoverable. config <- tryCatch({ reticulate::py_config() }, error = function(err) { @@ -68,7 +68,7 @@ }) if (inherits(ipykernel, "error")) { - return(list(python = python, venv = venv, error = as.character(ipykernel))) + return(list(python = python, venv = venv, error = conditionMessage(ipykernel))) } list( From 286a24aa0000ba736694dd133e9de117ba9d7d20 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Thu, 26 Sep 2024 11:25:57 -0300 Subject: [PATCH 15/16] Improvements to style and simplifications --- crates/ark/src/reticulate.rs | 80 ++++++++++++------------------------ 1 file changed, 26 insertions(+), 54 deletions(-) diff --git a/crates/ark/src/reticulate.rs b/crates/ark/src/reticulate.rs index 082448788..c3fb09b83 100644 --- a/crates/ark/src/reticulate.rs +++ b/crates/ark/src/reticulate.rs @@ -1,25 +1,24 @@ +use std::ops::Deref; +use std::sync::LazyLock; use std::sync::Mutex; use amalthea::comm::comm_channel::CommMsg; use amalthea::comm::event::CommManagerEvent; use amalthea::socket::comm::CommInitiator; use amalthea::socket::comm::CommSocket; -use anyhow::anyhow; use crossbeam::channel::Sender; use harp::RObject; -use lazy_static::lazy_static; use libr::R_NilValue; use libr::SEXP; use serde_json::json; +use stdext::result::ResultOrLog; use stdext::spawn; use stdext::unwrap; use uuid::Uuid; use crate::interface::RMain; -lazy_static! { - static ref RETICULATE_COMM_ID: Mutex> = Mutex::new(None); -} +static RETICULATE_COMM_ID: LazyLock>> = LazyLock::new(|| Mutex::new(None)); pub struct ReticulateService { comm: CommSocket, @@ -40,14 +39,15 @@ impl ReticulateService { }; let event = CommManagerEvent::Opened(service.comm.clone(), serde_json::Value::Null); - unwrap!(service.comm_manager_tx.send(event), Err(e) => { - log::error!("Reticulate: Could not open comm. Error {e}"); - }); + service + .comm_manager_tx + .send(event) + .or_log_error("Reticulate: Could not open comm."); spawn!(format!("ark-reticulate-{}", comm_id), move || { - unwrap!(service.handle_messages(), Err(err) => { - log::error!("Connection Pane: Error while handling messages: {err:?}"); - }); + service + .handle_messages() + .or_log_error("Reticulate: Error handling messages"); }); Ok(comm_id) @@ -61,33 +61,19 @@ impl ReticulateService { }); if let CommMsg::Close = msg { - self.comm.outgoing_tx.send(CommMsg::Close).unwrap(); break; } - - // Forward data msgs to the frontend - if let CommMsg::Data(_) = msg { - self.comm.outgoing_tx.send(msg)?; - continue; - } } // before finalizing the thread we make sure to send a close message to the front end - if let Err(err) = self.comm.outgoing_tx.send(CommMsg::Close) { - log::error!("Reticulate: Error while sending comm_close to front end: {err:?}"); - } - - let mut comm_id_guard = unwrap!( - RETICULATE_COMM_ID.try_lock(), - Err(e) => { - return Err(anyhow!("Could not access comm_id. Error {}", e)); - } - ); - log::info!( - "Reticulate Thread closing {}", - (*comm_id_guard).clone().unwrap() - ); - + self.comm + .outgoing_tx + .send(CommMsg::Close) + .or_log_error("Reticulate: Could not send close message to the front-end"); + + // Reset the global comm_id before closing + let mut comm_id_guard = RETICULATE_COMM_ID.lock().unwrap(); + log::info!("Reticulate Thread closing {:?}", (*comm_id_guard).clone()); *comm_id_guard = None; Ok(()) @@ -102,25 +88,16 @@ impl ReticulateService { pub unsafe extern "C" fn ps_reticulate_open(input: SEXP) -> Result { let main = RMain::get(); - if !RMain::initialized() { - return Ok(R_NilValue); - } - - let input = RObject::try_from(input)?; + let input: RObject = input.try_into()?; let input_code: Option = input.try_into()?; - // If there's an id already registered, we just need to send the focus event - let mut comm_id_guard = unwrap!( - RETICULATE_COMM_ID.try_lock(), - Err(e) => { - return Err(anyhow!("Could not access comm_id. Error {}", e)); - } - ); + let mut comm_id_guard = RETICULATE_COMM_ID.lock().unwrap(); - if let Some(id) = (*comm_id_guard).clone() { + // If there's an id already registered, we just need to send the focus event + if let Some(id) = comm_id_guard.deref() { // There's a comm_id registered, we just send the focus event main.get_comm_manager_tx().send(CommManagerEvent::Message( - id, + id.clone(), CommMsg::Data(json!({ "method": "focus", "params": { @@ -133,13 +110,8 @@ pub unsafe extern "C" fn ps_reticulate_open(input: SEXP) -> Result { - log::error!("Reticulate: Failed to start connection: {err:?}"); - return Err(err); - } - ); + + ReticulateService::start(id, main.get_comm_manager_tx().clone())?; Ok(R_NilValue) } From 8bf745fb29127286459a2fa676104e016c83bea6 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Thu, 26 Sep 2024 12:05:53 -0300 Subject: [PATCH 16/16] Add some comments + install packages rpc method --- crates/ark/src/modules/positron/package.R | 11 +++++++++++ crates/ark/src/modules/positron/reticulate.R | 17 +++++------------ 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/crates/ark/src/modules/positron/package.R b/crates/ark/src/modules/positron/package.R index 1c5099011..105f6743c 100644 --- a/crates/ark/src/modules/positron/package.R +++ b/crates/ark/src/modules/positron/package.R @@ -21,6 +21,17 @@ #' @export .ps.rpc.is_installed <- .ps.is_installed +#' @export +.ps.rpc.install_packages <- function(packages) { + for (pkg in packages) { + if (.ps.rpc.isPackageAttached(pkg)) { + stop("Should not install a package if it's already attached.") + } + } + utils::install.packages(packages) + TRUE +} + #' @export .ps.rpc.isPackageAttached <- function(pkg) { if (!is_string(pkg)) { diff --git a/crates/ark/src/modules/positron/reticulate.R b/crates/ark/src/modules/positron/reticulate.R index 41169b8eb..ae31fa1ea 100644 --- a/crates/ark/src/modules/positron/reticulate.R +++ b/crates/ark/src/modules/positron/reticulate.R @@ -3,18 +3,6 @@ .ps.Call("ps_reticulate_open", input) } -#' Used by the front-end to install reticulate -#' A modal asking to install is shown before calling this. -#' @export -.ps.rpc.install_reticulate <- function() { - tryCatch({ - utils::install.packages("reticulate") - TRUE - }, error = function(err) { - FALSE - }) -} - #' Called by the front-end right before starting the reticulate session. #' #' At this point it should be fine to load Python if it's not loaded, and @@ -81,6 +69,11 @@ #' @export .ps.rpc.reticulate_start_kernel <- function(kernelPath, connectionFile, logFile, logLevel) { + # Starts an IPykernel in a separate thread with information provided by + # the caller. + # It it's essentially executing the kernel startup script: + # https://github.com/posit-dev/positron/blob/main/extensions/positron-python/python_files/positron/positron_language_server.py + # and passing the communication files that Positron Jupyter's Adapter sets up. tryCatch({ reticulate:::py_run_file_on_thread( file = kernelPath,