feat: extension for remote R session management #24
Conversation
…n to handle R sessions properly
…e not having session support)
There was a problem hiding this comment.
Pull Request Overview
This PR implements remote R session management functionality for DataSHIELD, adding the ability to create, check, and monitor remote R sessions asynchronously. It introduces a new DSSession class and associated methods for session state management.
- Adds
DSSessionclass and related generics (dsIsReady,dsStateMessage,dsHasSession,dsSession) - Implements
datashield.sessions()function for managing multiple connections with async support and progress tracking - Integrates session management into existing DataSHIELD workflows (login, assign, aggregate operations)
Reviewed Changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| R/DSSession.R | New file defining DSSession class and core session management generics |
| R/DSConnection.R | Extended with session-related methods and improved documentation |
| R/datashield.sessions.R | New session management function with async support and callback handling |
| R/datashield.login.R | Simplified login logic and integrated workspace restoration |
| R/datashield.assign.R | Added session initialization calls to assignment functions |
| R/datashield.aggregate.R | Added session initialization call to aggregate function |
| R/datashield.workspace.R | Added session initialization to workspace operations |
| R/datashield.symbol.R | Added session initialization to symbol listing |
| man/*.Rd | Generated documentation files for new functions and updated cross-references |
| NAMESPACE | Exported new session-related functions and classes |
| DESCRIPTION | Updated collate order to include new session files |
Comments suppressed due to low confidence (1)
R/datashield.assign.R:272
- The
datashield.assign.resourcefunction is missing adatashield.sessions(conns)call at the beginning, which is inconsistent with other assignment functions likedatashield.assign.tableanddatashield.assign.expr.
datashield.assign.resource <- function(conns, symbol, resource, async=TRUE, success=NULL, error=NULL, errors.print = getOption("datashield.errors.print", FALSE)) {
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
timcadman
left a comment
timcadman
left a comment
There was a problem hiding this comment.
Looks good Yannick. Only suggestion is to consider refactoring datashield.session to reduce repetition and make it a bit more readable. I know this is in the same style as other DSI functions, so I leave this as an optional suggestion for you
| #' } | ||
| #' | ||
| #' @export | ||
| datashield.sessions <- function(conns, async=TRUE, success=NULL, error=NULL, errors.print = getOption("datashield.errors.print", FALSE)) { |
There was a problem hiding this comment.
Could this be refactored? These nested ifs within the for loop are quite hard to read and difficult to debug if something goes wrong.
| tryCatch({ | ||
| sessions[[n]] <- dsSession(fconns[[n]], async=async) | ||
| }, error = function(e) { | ||
| .appendError(n, conditionMessage(e)) |
There was a problem hiding this comment.
This is repeated - could you break into a separate helper function which is then called in the error part of the trycatch?
Implementation of #23