Restore cwd after do_command error WIP

[maedoc]

Submission Checklist

• Run unit tests
• Declare copyright holder and open-source license: see below

Summary

Addresses #506.

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): myself

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

• Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)

[maedoc]

It would be better if pushd was a proper context manager which handled exceptions by restoring cwd but this is easier and fixes the issue at hand. The only other use of pushd is in the install cmdstan/toolchain functions.

[codecov-commenter]

Codecov Report

Merging #507 (0e5c378) into develop (68ae4e7) will increase coverage by 0.43%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop     #507      +/-   ##
===========================================
+ Coverage    78.84%   79.27%   +0.43%     
===========================================
  Files           30       30              
  Lines         9288     6264    -3024     
===========================================
- Hits          7323     4966    -2357     
+ Misses        1965     1298     -667     

Impacted Files	Coverage Δ
...tanpy/cmdstanpy/cmdstanpy/install_cxx_toolchain.py
cmdstanpy/cmdstanpy/stanfit.py
...ner/work/cmdstanpy/cmdstanpy/cmdstanpy/progress.py
...k/cmdstanpy/cmdstanpy/cmdstanpy/install_cmdstan.py
...ner/work/cmdstanpy/cmdstanpy/cmdstanpy/__init__.py
...ner/work/cmdstanpy/cmdstanpy/cmdstanpy/_version.py
...work/cmdstanpy/cmdstanpy/cmdstanpy/cmdstan_args.py
...nner/work/cmdstanpy/cmdstanpy/cmdstanpy/stanfit.py
...ork/cmdstanpy/cmdstanpy/cmdstanpy/compiler_opts.py
...runner/work/cmdstanpy/cmdstanpy/cmdstanpy/model.py
... and 16 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 0b50ae9...0e5c378. Read the comment docs.

[WardBrian]

You can use try/except blocks inside a contextlib.contextmanager function like pushd. I think replacing the yield in pushd with

try:
    yield
except Exception:
    raise
finally:
    os.chdir(previous_dir)

would resolve the issue and be much more general

We can also then test pushd directly:

dir = os.getcwd()
with self.AssertRaises(RuntimeError):
    with pushd('data'):
        raise RuntimeError('testing')
self.assertPathsEqual(dir, os.getcwd())

[ahartikainen]

Yes I think adding

try:
    yield
finally:
    os.chdir(previous_dir)

is needed

cmdstanpy/cmdstanpy/utils.py

Line 1317 in 5b80eb8

@contextlib.contextmanager

[WardBrian]

I think it would be better to make the changes to pushd directly. We can then test the behavior for that directly (though there's no harm leaving the test for do_command)

[WardBrian]

@maedoc Let me know if you'd prefer if myself or Ari take the PR from here in terms of changes. Thanks for finding this issue and jumping on it so quickly!

[maedoc]

All the test failures are trying to compile the model with CmdStan on macOS. The error is the same as when I try to use a CmdStan build for x86_64 on Rosetta on macOS from Python running on arm64, or vice versa. Perhaps the CI script needs to be updated to check arch and rebuild CmdStan accordingly.

Also, improving CmdStanPy's support for the architecture issue on macOS would be helpful at some point.

[WardBrian]

I believe the error is because GitHub Actions updated the version of MacOs used but is caching old cmdstans. I’ll look into it

Thanks for the changes! I believe that with assertRaises there’s no need for a try/except in the test code. Other than that it looks great and I’ll merge once I fix the CI issues

[maedoc]

I believe that with assertRaises there’s no need for a try/except in the test code.

Some static analysers don't know that self.assertRaises catches exceptions and see the subsequent code as unreachable. Up to you..

[WardBrian]

We're not using try/except anywhere else in the tests where we use assertRaises, so for consistency I think don't here

[WardBrian]

The one test failure is from a timeout (I think we tried to download cmdstan too many times in short succession) so this is good to go

[WardBrian]

Thanks again @maedoc !