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

Create a background R session and keep running code in it #56

Merged
merged 22 commits into from Jun 4, 2018

Conversation

Projects
None yet
5 participants
@gaborcsardi
Copy link
Member

gaborcsardi commented May 25, 2018

This is basically ready, but it does need more testing, especially when sg fails, e.g. the remote R crashes.

gaborcsardi added some commits May 25, 2018

r_session$wait -> wait_for_call, avoid process$wait clash
processx::process already has a wait method...

@gaborcsardi gaborcsardi requested review from hadley , jimhester and lionel- May 25, 2018

@gaborcsardi gaborcsardi changed the title Keep a background R session and keep running code in it Create a background R session and keep running code in it May 25, 2018

@gaborcsardi

This comment has been minimized.

Copy link
Member Author

gaborcsardi commented May 25, 2018

❯ bench::mark("callr::r" = h(), "callr::r_session" = f(), "callr::r_session2" = f2(), local = g(), check = FALSE)
# A tibble: 4 x 14
  expression           min     mean   median      max `itr/sec` mem_alloc  n_gc
  <chr>           <bch:tm> <bch:tm> <bch:tm> <bch:tm>     <dbl> <bch:byt> <dbl>
1 callr::r        165.47ms 170.33ms 170.09ms 175.44ms    5.87e0    45.9KB     0
2 callr::r_sessi…   5.42ms   5.93ms   5.79ms   7.25ms    1.69e2    13.8KB     0
3 callr::r_sessi…   3.21ms   3.55ms   3.41ms   5.67ms    2.81e2      704B     1
4 local              184ns 281.17ns    203ns   5.47µs    3.56e6        0B     0
# ... with 6 more variables: n_itr <int>, total_time <bch:tm>, result <list>,
#   memory <list>, time <list>, gc <list>

The first r_session is the current implementation. The second is a better future implementation, without temporary files on the disk. Its running time is estimated here. local is an ordinary R function call.

@hadley
Copy link
Member

hadley left a comment

Is there a way for the process to know who it's parent is?

#' has started, and the elapsed time since the current computation has
#' started. The latter is NA if there is no active computation.
#'
#' `rs$get_state()` return the state of the R session. Possible values:

This comment has been minimized.

@hadley

hadley May 25, 2018

Member

Do we need a way to get the status code if the process has finished?

This comment has been minimized.

@gaborcsardi

gaborcsardi May 25, 2018

Author Member

It inherits all methods from processx::process, so you have rs$get_exit_status().

@gaborcsardi

This comment has been minimized.

Copy link
Member Author

gaborcsardi commented May 25, 2018

Is there a way for the process to know who it's parent is?

At the system level yes, but I don't think that we have an R API for it. Would that be useful?

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 25, 2018

Codecov Report

Merging #56 into master will decrease coverage by 6.19%.
The diff coverage is 83.39%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #56     +/-   ##
=========================================
- Coverage   97.17%   90.98%   -6.2%     
=========================================
  Files          15       16      +1     
  Lines         390      610    +220     
=========================================
+ Hits          379      555    +176     
- Misses         11       55     +44
Impacted Files Coverage Δ
R/r-process.R 95.83% <ø> (-4.17%) ⬇️
R/setup.R 96.15% <100%> (-1.38%) ⬇️
R/utils.R 90.62% <100%> (+3.12%) ⬆️
R/result.R 91.11% <100%> (-1.05%) ⬇️
R/r-session.R 80.43% <80.43%> (ø)
R/script.R 91.46% <88.88%> (-6.27%) ⬇️
R/options.R 100% <0%> (ø) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b092d9...5244bea. Read the comment docs.

@gaborcsardi

This comment has been minimized.

Copy link
Member Author

gaborcsardi commented May 25, 2018

One question here is what to do with standard output and standard error (by default). They probably should not be ignored, so we can write them to a file, or to a pipe. The pipe is easier to handle in the parent, but the parent needs to read it out, otherwise the child stops. And then it is easy to create a deadlock.

@lionel-

This comment has been minimized.

Copy link
Member

lionel- commented May 25, 2018

Would it make sense to leave that up to the function passed to the process? e.g. it could be wrapped with purrr::quietly(). This would assume that only warnings and messages write to stderr. Would it be possible to reliably capture stderr without going through sink()?

while (1) {
data <- processx::conn_write(con, data)
if (!length(data)) break;
Sys.sleep(.1)

This comment has been minimized.

@jimhester

jimhester May 25, 2018

Member

I wonder if the sleep time here and on L274 should be a parameter of the object?

This comment has been minimized.

@gaborcsardi

gaborcsardi May 25, 2018

Author Member

Could be, but this sleep is actually not very important. It is a safety net that is almost never needed. Basically it is only needed if the write buffer is full, but the write buffer is at least 4k or 8k, and we don't send messages longer than that, and only ever keep at most one message in the buffer. So it is not really important to make this user configurable, I think.


rs_finish <- function(self, private, grace) {
close(self$get_input_connection())
self$poll_io(grace)

This comment has been minimized.

@jimhester

jimhester May 25, 2018

Member

Is 200 (ms?) enough time to ensure the everything closes gracefully?

This comment has been minimized.

@gaborcsardi

gaborcsardi May 25, 2018

Author Member

200ms, yes. Should be, unless the R session has .Last which takes longer to run. For this odd case, the grace period is user configurable.

rs_finish <- function(self, private, grace) {
close(self$get_input_connection())
self$poll_io(grace)
self$kill()

This comment has been minimized.

@jimhester

jimhester May 25, 2018

Member

What signal does this send to the child process? SIGKILL, SIGTERM, SIGINT or SIGQUIT?

This comment has been minimized.

@gaborcsardi

gaborcsardi May 25, 2018

Author Member

SIGKILL On windows there is nothing else, and then it is better to behave the same way across platforms.

}

rs__write_for_sure <- function(self, private, text) {
while (1) {

This comment has been minimized.

@jimhester

jimhester May 25, 2018

Member

Maybe this could be

while(length(self$write_input(text)) > 0) {
  Sys.sleep(.1)
}

Although perhaps you feel that obscures what function is doing the work.

This comment has been minimized.

@gaborcsardi

gaborcsardi May 25, 2018

Author Member

You need to save the return value of write_input(), though. That's how write_input() works: it writes as much as it can, and returns the leftover that it could not write.

This comment has been minimized.

@jimhester

jimhester May 25, 2018

Member

Oh I see, so you would need to do something like

while(length(text <- self$write_input(text)) > 0) {
  Sys.sleep(1)
}

Which isn't much of an improvement, so it is fine as is.

@gaborcsardi

This comment has been minimized.

Copy link
Member Author

gaborcsardi commented May 25, 2018

Would it make sense to leave that up to the function passed to the process?

You need to select where the stdout and stderr go when you create the process.

I would avoid using sinks, because that's just another weak link, it makes the child logic more difficult, and I don't think it buys you much. Ultimately you want to be able to get the stdout and stderr in the parent, and either a file or a pipe is a better solution for that.

@gaborcsardi

This comment has been minimized.

Copy link
Member Author

gaborcsardi commented May 25, 2018

@lionel-

This would assume that only warnings and messages write to stderr

We already catch the errors and pass them to the parent, but we should probably catch the warnings as well.

@gaborcsardi

This comment has been minimized.

Copy link
Member Author

gaborcsardi commented May 25, 2018

So, after some thinking, I think by default we could write stdout and stderr to temp files. Then after a run or call + get_result we can just read the files from the right position to the end, to get the stdout and stderr of the evaluated function.

The only glitch with this solution, is that the files will forever grow on the disk, until the session is done. But I don't think we can do much about that.

In the (distant?) future, when we'll have a proper event loop running on a background thread, we can write stdout and stderr to pipes, and then the event loop thread will read them out regularly in the background, without bothering the main R thread.

gaborcsardi added some commits May 26, 2018

Catch errors, interrupts as well
This does not really matter for the one-shot
functions (except that their exit code is now
different), but it does matter for r_session.

@gaborcsardi gaborcsardi force-pushed the feature/session branch from 7851718 to 24cc242 May 27, 2018

@lionel-

This comment has been minimized.

Copy link
Member

lionel- commented May 28, 2018

About capturing conditions, I've been thinking about a maybe object that you could deref() (either get the value or rethrow conditions) or purrr::modify() (change the result unless the error field is active, i.e. Haskell's fmap). maybe would be the return value of functions modified with purrr::safely(). I'm not sure how warnings and messages would fit with this yet, either a different object type or having optional maybe fields:

maybe <- function(expr, error = TRUE, warnings = FALSE, messages = FALSE) {
  ...
}

x <- maybe({ warning("foo"); stop("bar") }, warnings = TRUE)
x
#> <maybe>
#> $result
#> NULL
#>
#> $error
#> <simpleError in force(expr): bar>
#>
#> $warnings
#> $warnings[[1]]
#> <simpleWarning in force(expr): foo>

has_failed(x)
#> [1] TRUE

deref(x)
#> Error: bar
#> In addition: Warning message:
#> foo

identical(x, purrr::modify(x, toupper))
#> [1] TRUE

Maybe that could be the return value of process calls?

@gaborcsardi

This comment has been minimized.

Copy link
Member Author

gaborcsardi commented May 28, 2018

@lionel- For this we need to change API, so we would need to introduce new functions. Which is fine, but I am wondering if the higher level utils belong to some other package, and if we should keep callr close to the system calls. E.g. callr could just return the result / error, the warnings, the stdout and strerr.

@gaborcsardi

This comment has been minimized.

Copy link
Member Author

gaborcsardi commented May 31, 2018

OK, now we use separate temporary files for each $call() to get the stdout and stderr. It turns out that it is possible to re-route stdout and stderr on the fly, and processx now supports this.

gaborcsardi added some commits May 31, 2018

@gaborcsardi gaborcsardi force-pushed the feature/session branch from 2554d31 to b2f1c31 May 31, 2018

gaborcsardi added some commits Jun 2, 2018

Use processx master
It has everything merged now.
Session: set process to idle on error
When we read out the result.
r_session: better error messages
When session is not in the right state for an
operation.
r_session: get_state() checks if alive
In case it crashed, or was $kill()-ed.
@gaborcsardi

This comment has been minimized.

Copy link
Member Author

gaborcsardi commented Jun 4, 2018

I'll merge this now, can continue to improve....

@gaborcsardi gaborcsardi merged commit 0bc8610 into master Jun 4, 2018

4 of 6 checks passed

codecov/patch 83.39% of diff hit (target 97.17%)
Details
codecov/project 90.98% (-6.2%) compared to 9b092d9
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@gaborcsardi gaborcsardi referenced this pull request Jun 4, 2018

Closed

r_session #54

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment