search

brain-microstructure-exploration-tools / abcd-microstructure-pipelines

Processing pipelines to extract brain microstructure from ABCD Study dMRI
Apache License 2.0
0 stars 1 forks source link

26 add more tests for brain masking #32

Closed ebrahimebrahim closed 4 months ago

ebrahimebrahim commented 4 months ago

Close #26

One thing I noticed while testing is that everything is very dependent on the file extensions being .nii.gz.

This can make it difficult to later extend to other possible input and output formats. Even the uncompressed .nii would currently fail some tests.

I wonder if we should try to better follow the principle of keeping IO separate and cordon off the IO aspects of this package into its own module? This also feels like work that is likely already done somewhere but I am not sure where to look...

ebrahimebrahim commented 4 months ago

The macos python3.9 CI failure is due to this. We chose macos-latest here and it seems that that macos-latest has changed such that it now points to macos-14-arm64. Python 3.9 isn't supported by the setup-python action on the platform macos-14-arm64.

ebrahimebrahim commented 4 months ago

The macos CI failure should be resolved once #33 is merged and this is rebased.

The failing tests for windows have me puzzled. With python3.9 we're getting

FAILED tests/test_masks.py::test_gen_b0_mean[nii] - NotADirectoryError: [WinError 267] The directory name is invalid: 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmppw9w_z45\\out.nii'

while with python3.12 we're getting

FAILED tests/test_masks.py::test_gen_b0_mean[nii] - PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmp5jx186md\\out.nii'

Looking into it...

ebrahimebrahim commented 4 months ago

@allemangD It looks like the failing tests on windows are struggling with the cleanup steps that come at the end of the TemporaryDirectory context manager. Do you know of anything that I should be watching out for on windows when using a TemporaryDirectory and saving/loading files from it?

allemangD commented 4 months ago

Do you know of anything that I should be watching out for on windows when using a TemporaryDirectory and saving/loading files from it?

I know you can't open a temporary file multiple times on windows since it aggressively locks things... wonder if there is some similar restriction to contents of directories? But I didn't notice any open() anywhere outside a with.

The message about being used by another process makes me wonder if cleanup is occurring out of order, or cleanup occurs while HD-BET is still doing something, or that HD-BET tries to open the file while the writer still has it open. (checked logs - this occurs on every TemporaryDwiFiles cleanup, even on non-hd-bet tests)

Could tests be running in parallel and somehow using the same temporary directory?