Fix handling of wsl output paths #1214
Conversation
| self$model_variables <- model_variables | ||
| self$save_cmdstan_config <- save_cmdstan_config | ||
| if (os_is_wsl()) { | ||
| if (os_is_wsl() && is.null(output_dir)) { |
There was a problem hiding this comment.
We used to convert output_dir immediately with wsl_safe_path(), so e.g. C:/output would become /mnt/c/output before the output filenames were created. Now we do the special WSL handling when output_dir is omitted.
compose_all_args() already converts output, diagnostic, and profile filenames with wsl_safe_path(), so CmdStan still gets the right paths.
| # Need to update the output directory path to one that can be accessed | ||
| # from Windows, for the post-processing of results | ||
| self$runset$args$output_dir <- wsl_safe_path(self$runset$args$output_dir, revert = TRUE) |
There was a problem hiding this comment.
Unnecessary now. Also, previously this would only run after the CmdStanRun object had already made all the output filenames, so this would only fix args$output_dir, but not the $output_files() and other similar methods.
| }, | ||
| config_files = function(include_failed = FALSE) { | ||
| files <- private$config_files_ | ||
| files_win_path <- sapply(private$config_files_, wsl_safe_path, revert = TRUE) |
There was a problem hiding this comment.
files_win_path is never actually used anywhere after it's created. Unnecessary now, anyway.
| withr::local_tempdir(pattern = pattern, .local_envir = .local_envir) | ||
| } | ||
|
|
||
| test_that("WSL output paths stay host-native until command composition", { |
There was a problem hiding this comment.
I got Claude to help me with these tests! I reviewed them and I think I understand what they're doing.
|
@andrjohns do you mind taking a look at this? very few actual code changes, but it touches WSL which you know much better than I do. Maybe there's an unintended consequence I'm overlooking. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1214 +/- ##
==========================================
+ Coverage 91.90% 92.09% +0.19%
==========================================
Files 15 15
Lines 6260 6249 -11
==========================================
+ Hits 5753 5755 +2
+ Misses 507 494 -13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Submission Checklist
Summary
Fixes #1110.
(Note: I had Claude code assist with writing the tests for this because I don't have a WSL locally right now. )
With this PR we now keep CmdStan output paths in the native Windows/R namespace when using WSL, and we only translate them to WSL paths when constructing CmdStan commands (
compose_all_argsalready handles this). This ensures output files (and diagnostic, profile, config, and metric files) work with functions likefile.exists(),save_*_files(), etc.Copyright and Licensing
Please list the copyright holder for the work you are submitting
(this will be you or your assignee, such as a university or company):
Jonah Gabry
By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses: