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

FIX: extend read/execute permissions to all files used by BIBSnet #134

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

scott-huberty
Copy link
Contributor

@scott-huberty scott-huberty commented Sep 5, 2024

Fixes #132
Fixes #133

Since the devs are already giving all users read+execute permissions to all files in /home/bibsnet/src, I am assuming that it is okay with the devs to extend this to /home/bibsnet/data and /opt/nnUNet


Long story:

Both of the linked issues arose from file permission issues, specifically for users who are in the others permission group (i.e. not in the user or group groups).

For example, on main, if you enter into the docker container with:

docker run -it --entrypoint /bin/bash dcanumn/bibsnet:latest

OR

singularity shell --nv --cleanenv --no-home /home/path/to/bibsnet.sif

These are the permissions for /home/bibsnet/data:

ls -l /home/bibsnet/data
drwxrws--- 2 77661 12710 4096 Aug 26  2022 MNI_templates
-rwxrwx--- 1 77661 12710 1370 Aug 26  2022 age_to_avg_head_radius_BCP.csv
-rwxrwx--- 1 77661 12710  295 Aug 26  2022 dataset_description.json
-rwxrwx--- 1 77661 12710   32 Aug 26  2022 identity_matrix.mat
drwxrws--- 2 77661 12710 4096 Aug 26  2022 look_up_tables
-rwxrwx--- 1 77661 12710  713 Aug 14 16:28 models.csv
-rwxrwx--- 1 77661 12710  295 Mar 28 21:29 sidecar_template.json

And:

ls -l /opt/nnUNet/nnUNet_raw_data_base/nnUNet_trained_models/nnUNet/3d_fullres/

In the docker image I get:

drwxrwsr-- 3 77661 12710 4096 Aug 15 15:19 Task540
drwxrwsr-- 3 77661 12710 4096 Aug 15 15:18 Task541
drwxrwsr-- 3 77661 12710 4096 Aug 15 15:18 Task542

But in the singularity image I get:

d????????? ? ? ? ?            ? Task540

This was a really nasty one to debug.... I think it might be worth including a test that asserts that all files needed by BIBSnet give read+execute permissions to all users, to avoid a code regression in the future.

…snet

Since the devs are already giving all users read and execute permissions to all directories and files in the /home/bibsnet/src folders, the /home/bibsnet/bibsnet folders, and in /home/bibsnet/run.py - I am assuming that it is okay with the devs to extend this to /home/bibsnet/data and /opt/nnUNeti

- I used the symolic chmod syntax as it is more readable in my opinion than e.g. 555
- Fixes DCAN-Labs#132
- Fixes DCAN-Labs#133

Both the linked issues arose from users beloning to the 'other' group having no file permissions (at least when using singularity).

- home/bibsnet/data/models.csv was unreadable for these users, which would cause bibsnet to crash.
- the model directories (e.g. Task540, Task541, Task542) were not readable for these users, causing BIBSnet to crash with a error message that was hard to interpret. Basically since the user has no permissions to these tasks folders, then nnUNet will just crash with a message that it can't find the Task540, Task541, Task542 folder etc.
@LuciMoore
Copy link
Contributor

@scott-huberty thank you so much for working on this! makes sense that it was a permissions issue. I just want to confirm with @tjhendrickson before merging (he's currently OOO but will be back next week)

@scott-huberty
Copy link
Contributor Author

No problem! And on that note maybe we want to consider #86 before merging too? In which case we'd want to add a few lines of code to make sure we don't grant execute permissions to non-executable files.

@LuciMoore
Copy link
Contributor

Good point, thanks for the reminder. I'll work on getting this resolved next week!

@scott-huberty
Copy link
Contributor Author

No worries! We can incorporate #86 into this PR if it's easier. Happy to pick things up next week after @tjhendrickson is back in office.

@LuciMoore
Copy link
Contributor

@scott-huberty we've decided to hold off on #86 for now, but will merge your changes and tag a new release. thanks again for your work on this!

@LuciMoore LuciMoore merged commit acb9e6e into DCAN-Labs:main Sep 10, 2024
@scott-huberty scott-huberty deleted the permissions_fix branch September 10, 2024 20:16
@scott-huberty
Copy link
Contributor Author

No problem and thanks @LuciMoore ! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants