fix: resolve video save path without os.chdir (fixes #378)#448
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #448 +/- ##
==========================================
- Coverage 89.07% 88.90% -0.17%
==========================================
Files 27 27
Lines 1281 1280 -1
==========================================
- Hits 1141 1138 -3
- Misses 140 142 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Haris-bin-shakeel
force-pushed
the
fix/378-video-save-path
branch
2 times, most recently
from
743dafb to
588daad
Compare
|
Added |
Haris-bin-shakeel
force-pushed
the
fix/378-video-save-path
branch
from
e240ee8 to
39d4813
Compare
IgorTatarnikov
left a comment
IgorTatarnikov
left a comment
There was a problem hiding this comment.
Thanks @Haris-bin-shakeel, this looks great!
Just a small suggestion to use tmp_path in the existing tests to harmonise with the new test.
| def test_video(): | ||
| s = Scene(title="BR") | ||
|
|
||
| s.add_brain_region("TH") | ||
|
|
||
| vm = VideoMaker(s, "tests", "test") | ||
| savepath = vm.make_video(duration=1, fps=15, azimuth=3) | ||
|
|
||
| assert savepath == "tests/test.mp4" | ||
| assert savepath == str(Path("tests").resolve() / "test.mp4") | ||
| path = Path(savepath) | ||
| assert path.exists() | ||
| path.unlink() |
There was a problem hiding this comment.
Could this test be changed to use the tmp_path directory, instead of replicating the features with path.unlink().
There was a problem hiding this comment.
Thanks for the suggestion, @IgorTatarnikov! I agree that using tmp_path is much cleaner. I've updated the existing tests to use the fixture to match the new test, removing the manual file cleanup. The changes are pushed and ready for your review!
| assert savepath == str(Path("tests").resolve() / "test.mp4") | ||
| path = Path(savepath) | ||
| assert path.exists() | ||
| path.unlink() |
There was a problem hiding this comment.
Similar here, could this test be changed to use the tmp_path fixture?
There was a problem hiding this comment.
Done here as well!
IgorTatarnikov
left a comment
IgorTatarnikov
left a comment
There was a problem hiding this comment.
Not sure why the CI tests are not running, but I've double checked the tests pass locally and the video example generates the files in the expected directory.
2 participants
Summary
Fixes #378 by removing
os.chdir()fromVideoMaker.make_video()and switching to explicit absolute paths throughout the video pipeline.Root Cause
make_video()calledos.chdir(self.save_fld)so ffmpeg could locate the output file using only a bare filename. The CWD was restored at the end — but only on the happy path. Any exception during rendering left the process CWD permanently mutated, causing subsequent path operations in the caller's script to silently resolve to the wrong location. This explains the intermittent nature of the bug._video.pycompounded this by passing only a filename to ffmpeg, making output placement entirely dependent on global CWD state.Changes
brainrender/video.py: removedcurdir = os.getcwd()and bothos.chdir()calls;Video()now receives a fully-resolved absolute path viaself.save_fld.resolve();compress()updated to matchbrainrender/_video.py: ffmpeg input dir and output file both quoted to handle paths with spacestests/test_video.py: assertions updated to validate resolved absolute pathsVerification