-
Notifications
You must be signed in to change notification settings - Fork 234
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
Copy all run and out dirs at once, not in for-loop #3025
Conversation
Marked as draft again because I'm getting close (hopefully) to really fixing this. |
Ok, the remaining bug has nothing to do with PEcAn or with R for that matter. I've been able to reproduce it with just |
@robkooper could you look at the changes you requested so we can figure out whether this is ready to pull? |
base/remote/R/remote.copy.from.R
Outdated
@@ -45,5 +45,37 @@ remote.copy.from <- function(host, src, dst, options = NULL, delete = FALSE, std | |||
} | |||
} | |||
PEcAn.logger::logger.debug("rsync", shQuote(args)) | |||
system2("rsync", shQuote(args), stdout = TRUE, stderr = as.logical(stderr)) | |||
out <- | |||
system2("rsync", args, stdout = "", stderr = as.logical(stderr)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is removing the shQuote
around args
intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was intentional, but now that you bring it up I'm not sure it's correct. system2()
always shQuote
s command
, but I guess it doesn't do that for args
. I'll add it back and make sure everything still works.
base/remote/R/remote.copy.from.R
Outdated
out <- | ||
system2("rsync", args, stdout = "", stderr = as.logical(stderr)) | ||
|
||
# Informative errors from rsync man page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the desire for clarity, but I'd rather send people to the rsync docs for these error messages. We use whatever rsync version is installed on the host system, so hard-coding values here feels like an invitation for them to get out of sync.
I assume rsync doesn't often change the meaning of exit codes between versions, but if they add a new code that's not on this list then this switch will return an empty string instead of the numeric value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this makes sense. I'm going to keep errors (anything other than 0) as logger.severe()
though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested with rabbitmq. please address issues pointed out by @infotroph
Addresses #3019 with changes to
start_model_runs()
Description
remote.copy.to
outside of for loop to copy over all /out and /run directories at once.remote.copy.from
outside of for loop to copy all outputs back to local at once.remote.copy.to()
andremote.copy.from()
There aren't any tests for
start_model_runs()
and I'm not sure how to write them, but I did install this PR and test it with my own setup.Motivation and Context
Review Time Estimate
Types of changes
Checklist: