Hipercow Linux Support #168
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #168 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 27 27
Lines 2480 2481 +1
=========================================
+ Hits 2480 2481 +1 ☔ View full report in Codecov by Sentry. |
| template_file <- if (run_on_linux) "task_run.sh" else "task_run.bat" | ||
| out_script <- if (run_on_linux) SH_RUN else BATCH_RUN | ||
| write_os_lines <- if (run_on_linux) write_linux_lines else writeLines |
There was a problem hiding this comment.
This might be easier to read in one if/else
if (run_on_linux) {
template_file <- ...
out_script <- ...
} else {
}
etc
| path_dat <- prepare_path(path, config$shares) | ||
| linux_path <- path_on_linux(path_dat) | ||
| wrap_path <- path_to_task_file(path_root, task_id, SH_WRAP_RUN) | ||
| write_linux_lines( | ||
| sprintf("python -u /opt/hpcnodemanager/kwrap.py %s", linux_path), | ||
| wrap_path) |
There was a problem hiding this comment.
Can you add a comment explaining what any of this does please?
| path | ||
| } | ||
|
|
||
|
|
||
| write_batch_provision_script <- function(id, config, path_root) { | ||
| run_on_linux <- config$platform == "linux" | ||
| template_file <- if (run_on_linux) "provision.sh" else "provision.bat" |
There was a problem hiding this comment.
The other way of doing this is you have a list
details <- list(
linux = list(template_file = "provision.sh", write_lines = write_linux_lines),
windows = list(template_file = "provision.bat", write_lines = writeLines)
and then you do
template_file <- details[[config$platform]][["template_file"]]
or similar. But I think the version above is probably easiest tbh
| x <- prepare_path(x, shares) | ||
| file.path(x$drive_remote, x$rel) | ||
| if (run_on_linux) { | ||
| path_on_linux(x) |
There was a problem hiding this comment.
this name I think is insufficiently clear as to what sort of changes are going on here, and in particular I think that this is really "path on the dide network as seen from a linux machine", as opposed to some sort of windows/linux slash normalisation that one might expect
| @@ -36,10 +36,10 @@ web_client <- R6::R6Class( | |||
| }, | |||
|
|
|||
| submit = function(path, name, resources, cluster = NULL, | |||
| depends_on = NULL) { | |||
| depends_on = NULL, workdir = "") { | |||
There was a problem hiding this comment.
What is going on with workdir here? I'm not thrilled that we need to set it on linux but not windows and that is has a default -- which presumably won't work on linux
| client_parse_cluster_resources(httr_text(r)) | ||
| cluster_resources = function() { | ||
| r_windows <- private$client$GET( | ||
| "/api/v1/cluster_info/wpia-hn/hipercow.windows/", public = TRUE) |
There was a problem hiding this comment.
These names are immediately out of date as they look like package names (we had hipercow.windows but now we are working towards hipercow.dide. I know that the portal will be replaced by a new API soon but we might want to not bake in these names all the same
| module load hiredis | ||
| module load libsodium | ||
|
|
||
| echo working directory: $(pwd) |
There was a problem hiding this comment.
in the windows version the first bit of the script moves to the working directory. Is there some reason this won't work in linux? If not then we probably should move that bit of control into the same place on windows too
|
|
||
| module load R/{{r_version}} | ||
| module load hiredis | ||
| module load libsodium |
There was a problem hiding this comment.
what ends up needing libsodium?
No description provided.