juaml/junifer
Watch
3
0
Fork
You've already forked junifer
 0
Code Issues 14 Pull requests 2 Packages Activity Actions

[ENH]: Introduce WorkDirManager #254

Merged
synchon merged 30 commits from feat/workdirmanager into main 2023-10-13 08:15:05 +00:00
Conversation 8 Commits 30 Files changed 15 +535 -180
synchon commented 2023-10-04 16:52:43 +00:00 (Migrated from github.com)
Copy link
  • description of feature/fix
  • tests added/passed
  • add an entry for the latest changes

This PR introduces new WorkDirManager class which allows global access for temporary directories under the workdir parameter's values from the YAML, across the codebase.

* [x] description of feature/fix * [x] tests added/passed * [x] add an entry for the latest changes This PR introduces new `WorkDirManager` class which allows global access for temporary directories under the `workdir` parameter's values from the YAML, across the codebase.
codecov[bot] commented 2023-10-04 17:10:55 +00:00 (Migrated from github.com)
Copy link

Codecov Report

Merging #254 (d2f870b) into main (22ca06f) will decrease coverage by 0.23%.
Report is 23 commits behind head on main.
The diff coverage is 81.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #254      +/-   ##
==========================================
- Coverage   93.03%   92.81%   -0.23%     
==========================================
  Files          84       86       +2     
  Lines        3718     3771      +53     
  Branches      724      733       +9     
==========================================
+ Hits         3459     3500      +41     
- Misses        161      170       +9     
- Partials       98      101       +3
Flag Coverage Δ
docs 100.00% <ø> (ø)
junifer 92.81% <81.92%> (-0.23%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
junifer/api/functions.py 96.27% <100.00%> (+0.04%) ⬆️
junifer/markers/utils.py 90.47% <100.00%> (-2.39%) ⬇️
junifer/pipeline/__init__.py 100.00% <100.00%> (ø)
junifer/pipeline/singleton.py 100.00% <100.00%> (ø)
junifer/markers/falff/falff_estimator.py 95.45% <77.77%> (-0.06%) ⬇️
junifer/markers/reho/reho_estimator.py 68.02% <83.33%> (-0.69%) ⬇️
junifer/pipeline/workdir_manager.py 78.00% <78.00%> (ø)
## [Codecov](https://app.codecov.io/gh/juaml/junifer/pull/254?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report > Merging [#254](https://app.codecov.io/gh/juaml/junifer/pull/254?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (d2f870b) into [main](https://app.codecov.io/gh/juaml/junifer/commit/22ca06fe0872b83749ccc1de2917b549be4ffef4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (22ca06f) will **decrease** coverage by `0.23%`. > Report is 23 commits behind head on main. > The diff coverage is `81.92%`. [![Impacted file tree graph](https://app.codecov.io/gh/juaml/junifer/pull/254/graphs/tree.svg?width=650&height=150&src=pr&token=5H21JuZXMw&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml)](https://app.codecov.io/gh/juaml/junifer/pull/254?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #254 +/- ## ========================================== - Coverage 93.03% 92.81% -0.23% ========================================== Files 84 86 +2 Lines 3718 3771 +53 Branches 724 733 +9 ========================================== + Hits 3459 3500 +41 - Misses 161 170 +9 - Partials 98 101 +3 ``` | [Flag](https://app.codecov.io/gh/juaml/junifer/pull/254/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [docs](https://app.codecov.io/gh/juaml/junifer/pull/254/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | `100.00% <ø> (ø)` | | | [junifer](https://app.codecov.io/gh/juaml/junifer/pull/254/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | `92.81% <81.92%> (-0.23%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/juaml/junifer/pull/254?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [junifer/api/functions.py](https://app.codecov.io/gh/juaml/junifer/pull/254?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9hcGkvZnVuY3Rpb25zLnB5) | `96.27% <100.00%> (+0.04%)` | :arrow_up: | | [junifer/markers/utils.py](https://app.codecov.io/gh/juaml/junifer/pull/254?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL3V0aWxzLnB5) | `90.47% <100.00%> (-2.39%)` | :arrow_down: | | [junifer/pipeline/\_\_init\_\_.py](https://app.codecov.io/gh/juaml/junifer/pull/254?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9waXBlbGluZS9fX2luaXRfXy5weQ==) | `100.00% <100.00%> (ø)` | | | [junifer/pipeline/singleton.py](https://app.codecov.io/gh/juaml/junifer/pull/254?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9waXBlbGluZS9zaW5nbGV0b24ucHk=) | `100.00% <100.00%> (ø)` | | | [junifer/markers/falff/falff\_estimator.py](https://app.codecov.io/gh/juaml/junifer/pull/254?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL2ZhbGZmL2ZhbGZmX2VzdGltYXRvci5weQ==) | `95.45% <77.77%> (-0.06%)` | :arrow_down: | | [junifer/markers/reho/reho\_estimator.py](https://app.codecov.io/gh/juaml/junifer/pull/254?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL3JlaG8vcmVob19lc3RpbWF0b3IucHk=) | `68.02% <83.33%> (-0.69%)` | :arrow_down: | | [junifer/pipeline/workdir\_manager.py](https://app.codecov.io/gh/juaml/junifer/pull/254?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9waXBlbGluZS93b3JrZGlyX21hbmFnZXIucHk=) | `78.00% <78.00%> (ø)` | |
fraimondo (Migrated from github.com) requested changes 2023-10-05 11:30:36 +00:00
junifer/pipeline/workdir_manager.py
@ -0,0 +1,162 @@
"""Provide a work directory manager class to be used by pipeline components."""
fraimondo (Migrated from github.com) commented 2023-10-05 11:30:13 +00:00
Copy link

I would not allow to set the workdir like this, for the resasons stated in the constructor

I would not allow to set the workdir like this, for the resasons stated in the constructor
fraimondo (Migrated from github.com) commented 2023-10-05 11:30:28 +00:00
Copy link

no prefix here, the root tmp dir should be random.

no prefix here, the root tmp dir should be random.
junifer/pipeline/workdir_manager.py
@ -0,0 +38,4 @@
"""
def __init__(self, workdir: Optional[Union[str, Path]] = None) -> None:
fraimondo (Migrated from github.com) commented 2023-10-05 11:29:40 +00:00
Copy link

Since it's a singleton, any intialization should take care of cleaning up a previous one.

e.g:

WorkdirManager("/tmp/workdir1")
# Do some stuff that creates tempdirs
WorkdirManager("/tmp/workdir2")  # This should cleanup.

However, if a user does use it wrongly, it might cleanup in the middle.

So I would:

  1. add a cleanup=False parameter to the init
  2. Raise an error if needs cleanup but cleanup is False
Since it's a singleton, any intialization should take care of cleaning up a previous one. e.g: ``` WorkdirManager("/tmp/workdir1") # Do some stuff that creates tempdirs WorkdirManager("/tmp/workdir2") # This should cleanup. ``` However, if a user does use it wrongly, it might cleanup in the middle. So I would: 1) add a `cleanup=False` parameter to the init 2) Raise an error if needs cleanup but cleanup is False
github-actions[bot] commented 2023-10-05 16:42:34 +00:00 (Migrated from github.com)
Copy link
PR Preview Action v1.4.4
Preview removed because the pull request was closed.
2023-10-13 08:20 UTC
[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.4.4 :---: Preview removed because the pull request was closed. 2023-10-13 08:20 UTC <!-- Sticky Pull Request Commentpr-preview -->
synchon (Migrated from github.com) reviewed 2023-10-06 08:53:38 +00:00
junifer/pipeline/workdir_manager.py
@ -0,0 +38,4 @@
"""
def __init__(self, workdir: Optional[Union[str, Path]] = None) -> None:
synchon (Migrated from github.com) commented 2023-10-06 08:53:38 +00:00
Copy link

I've made the changes as you proposed. The problem is that the __init__() doesn't fire the second time you try to create an instance as it's a singleton, so the previous instance is returned.

I've made the changes as you proposed. The problem is that the `__init__()` doesn't fire the second time you try to create an instance as it's a singleton, so the previous instance is returned.
fraimondo (Migrated from github.com) reviewed 2023-10-10 09:20:08 +00:00
junifer/pipeline/workdir_manager.py
@ -0,0 +38,4 @@
"""
def __init__(self, workdir: Optional[Union[str, Path]] = None) -> None:
fraimondo (Migrated from github.com) commented 2023-10-10 09:20:08 +00:00
Copy link

Ok, so how can we fix it?

Ok, so how can we fix it?
synchon (Migrated from github.com) reviewed 2023-10-10 09:28:31 +00:00
junifer/pipeline/workdir_manager.py
@ -0,0 +38,4 @@
"""
def __init__(self, workdir: Optional[Union[str, Path]] = None) -> None:
synchon (Migrated from github.com) commented 2023-10-10 09:28:31 +00:00
Copy link

The only way I know of interacting with singletons in principle is to use getters and setters.

The only way I know of interacting with singletons in principle is to use getters and setters.
fraimondo (Migrated from github.com) reviewed 2023-10-10 09:36:54 +00:00
junifer/pipeline/workdir_manager.py
@ -0,0 +38,4 @@
"""
def __init__(self, workdir: Optional[Union[str, Path]] = None) -> None:
fraimondo (Migrated from github.com) commented 2023-10-10 09:36:54 +00:00
Copy link

lets do that then

lets do that then
fraimondo (Migrated from github.com) approved these changes 2023-10-13 07:05:06 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
fraimondo
Labels
Clear labels   
CRITICAL
High Priority Issue

  
Stale

   
WIP
Work in progress

  
bug
Something isn't working

  
concept
Design/Implementation concepts

  
coordinate
Issues or pull requests related to coordinates (ROIs)

  
dataset
Issues or pull requests related to datagrabbers or datasets

  
dependencies
Pull requests that update a dependency file

  
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
github_actions
Pull requests that update GitHub Actions code

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
maintenance
Issues or pull requests related to maintenance

  
maps
Issues or pull requests related to maps (probabilistic atlases)

  
marker
Issues or pull requests related to markers

  
mask
Issues or pull requests related to masks

  
on hold
Issue waiting on other issue(s)

  
parcellation
Issues or pull requests related to parcellations (deterministic atlases)

  
preprocess
Issues or pull requests related to preprocessors

  
question
Further information is requested

  
ready
Pull request is ready for review and merge

  
storage
Issues or pull requests related to marker storage or retrieval

  
template-space
Issues or pull requests related to template spaces

  
triage
New issues waiting to be reviewed

  
wontfix
This will not be worked on

No labels
CRITICAL
Stale
WIP
bug
concept
coordinate
dataset
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
maintenance
maps
marker
mask
on hold
parcellation
preprocess
question
ready
storage
template-space
triage
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
juaml/junifer!254
No description provided.