Skip to content
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

Allow executing cmdstan through WSL #677

Merged
merged 29 commits into from
Jul 17, 2022
Merged

Allow executing cmdstan through WSL #677

merged 29 commits into from
Jul 17, 2022

Conversation

andrjohns
Copy link
Collaborator

Submission Checklist

  • Run unit tests
  • Declare copyright holder and agree to license (see below)

Summary

This PR adds the ability to install cmdstan and run all models through WSL, all from a regular Windows R session.

The end-user simply needs to add wsl=TRUE to the install_cmdstan() call, and all model compilation and execution for that installation will subsequently be run through WSL. No other user-facing changes are made.

In addition to the known speed differences between Stan models under Windows & WSL, this also provides an accessible means for running MPI models in Windows

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):
Andrew Johnson

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses:

@andrjohns
Copy link
Collaborator Author

Forgot to mention in the description - this PR also adds a Github actions workflow to run the unit tests under WSL as well

@rok-cesnovar
Copy link
Member

WOW! This is awesome stuff and will be a huge benefit to Windows users! Fantastic!

I am going to review this later today and tomorrow in detail, just a few questions so I know I am understanding this correctly:

  • you can execute any command in WSL by running "wsl something.exe", is that correct? Has this been the case always?
  • The install location remains the same, but installs the WSL CmdStan's with a wsl- prefix?
  • WSL is used in a fresh session if the selected CmdStan installation path starts with wsl- or CMDSTANR_USE_WSL is set?

Lastly, please add yourself to the list of contributors.

@codecov-commenter
Copy link

codecov-commenter commented Jul 17, 2022

Codecov Report

Merging #677 (9f4e8a6) into master (85a504d) will decrease coverage by 1.92%.
The diff coverage is 48.38%.

@@            Coverage Diff             @@
##           master     #677      +/-   ##
==========================================
- Coverage   91.86%   89.94%   -1.93%     
==========================================
  Files          12       12              
  Lines        3540     3649     +109     
==========================================
+ Hits         3252     3282      +30     
- Misses        288      367      +79     
Impacted Files Coverage Δ
R/csv.R 97.66% <16.66%> (-1.05%) ⬇️
R/utils.R 79.74% <31.57%> (-9.37%) ⬇️
R/install.R 62.93% <34.00%> (-4.84%) ⬇️
R/path.R 71.05% <35.71%> (-8.32%) ⬇️
R/run.R 95.26% <70.58%> (-0.72%) ⬇️
R/args.R 98.65% <87.50%> (-0.19%) ⬇️
R/model.R 91.22% <95.45%> (+0.05%) ⬆️

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 85a504d...9f4e8a6. Read the comment docs.

Copy link
Member

@rok-cesnovar rok-cesnovar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and just reviewed. Ignore my questions above, I think I got all the answers from the code. The code change looks great and the tests are passing in all OSes. I just want to take a few minutes to test it live on my Windows machine before final approval.

I just have two comments, both are very minor and optional.

R/install.R Show resolved Hide resolved
R/path.R Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
Copy link
Member

@rok-cesnovar rok-cesnovar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again! Will update NEWS.md and bump the version to 0.5.3.

@rok-cesnovar rok-cesnovar merged commit e23f3a2 into stan-dev:master Jul 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants