Uh oh!
There was an error while loading. Please reload this page.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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?
Uh oh!
There was an error while loading. Please reload this page.
Implementation of #23