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

[BUG]: HDF5Storage fails to collect #198

Merged
fraimondo merged 9 commits from fix/hdf5_collect into main 2023-03-24 09:51:24 +00:00
Conversation 5 Commits 9 Files changed 3 +357 -209
fraimondo commented 2023-03-20 20:03:32 +00:00 (Migrated from github.com)
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

HCP results (4214 files) are not colected:

Traceback (most recent call last):
  File "/home/fraimondo/anaconda3/envs/junifer/bin/junifer", line 8, in <module>
    sys.exit(cli())
  File "/home/fraimondo/anaconda3/envs/junifer/lib/python3.9/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/home/fraimondo/anaconda3/envs/junifer/lib/python3.9/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/home/fraimondo/anaconda3/envs/junifer/lib/python3.9/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/fraimondo/anaconda3/envs/junifer/lib/python3.9/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/fraimondo/anaconda3/envs/junifer/lib/python3.9/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/home/fraimondo/dev/tbox/junifer/junifer/api/cli.py", line 192, in collect
    api_collect(storage=storage)
  File "/home/fraimondo/dev/tbox/junifer/junifer/api/functions.py", line 199, in collect
    storage_object.collect()
  File "/home/fraimondo/dev/tbox/junifer/junifer/storage/hdf5.py", line 862, in collect
    file_ = element_files[i]
IndexError: list index out of range

Closer inspection at the issue, it seems that the logic of chunking the collect is not correct:
github.com/juaml/junifer@85b5ff538d/junifer/storage/hdf5.py (L858-L874)

The problem is that the number of files is not multiple of the chunk_size, so the last chunk is not complete. This raises an error were i is 4214.

And, since we are fixing this logic, the nested progress bars are not correct. Indeed, this is how it looks:

file-data: 100%|█████████████████████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00,  6.24it/s]
file-data: 100%|█████████████████████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00,  6.13it/s]
file-data: 100%|█████████████████████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00,  6.18it/s]
file-data: 100%|█████████████████████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00,  6.23it/s]
file-data: 100%|█████████████████████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00,  6.17it/s]
file-data: 100%|█████████████████████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00,  6.20it/s]
file-data: 100%|█████████████████████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00,  6.09it/s]
file-data: 100%|█████████████████████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00,  6.19it/s]
file-data: 100%|█████████████████████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00,  6.16it/s]
file-data: 100%|█████████████████████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00,  6.08it/s]
chunk: 28it [08:00, 17.17s/it]███████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00,  6.24it/s]
feature:   0%|

This is because for the "chunk" progress bar, is not possible to know the size in an enumerate:

            for chunk_idx, chunk_start in tqdm(
                enumerate(range(0, element_count, chunk_size)), desc="chunk"
            ):

I would even suggest that we do not need separate progressbars due to the chunks. Indeed what we care is to show how the files are being included. So I would only keep 2 bars, one for the feature and one for the file.

Expected Behavior

Collect to run without issues.

Steps To Reproduce

Might need access to this specific project directory.

from junifer.storage import HDF5FeatureStorage

fname = "/data/project/SPP2041/results/fraimondo/brain_size_project/storage/HCP_epi_mask_falff/HCP_epi_mask_falff.hdf5"

storage = HDF5FeatureStorage(fname, single_output=False)
storage.collect()

Environment

junifer:
  version: 0.0.1.dev927
python:
  version: 3.9.16
  implementation: CPython
dependencies:
  click: 8.1.3
  numpy: 1.21.2
  datalad: 0.17.0+872.g8c7bc10dd
  pandas: 1.4.1
  nibabel: 3.2.2
  nilearn: 0.9.0
  sqlalchemy: 1.4.32
  yaml: '6.0'
system:
  platform: Linux-4.19.0-21-amd64-x86_64-with-glibc2.28
environment:
  LC_CTYPE: en_US.UTF-8
  PATH: /usr/lib/fsl/5.0:/home/fraimondo/anaconda3/envs/junifer/bin:/home/fraimondo/anaconda3/condabin:/home/fraimondo/.dotfiles/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/X11R6/bin:/usr/local/games:/usr/games

Relevant log output

No response

Anything else?

No response

### Is there an existing issue for this? - [X] I have searched the existing issues ### Current Behavior HCP results (4214 files) are not colected: ``` Traceback (most recent call last): File "/home/fraimondo/anaconda3/envs/junifer/bin/junifer", line 8, in <module> sys.exit(cli()) File "/home/fraimondo/anaconda3/envs/junifer/lib/python3.9/site-packages/click/core.py", line 1130, in __call__ return self.main(*args, **kwargs) File "/home/fraimondo/anaconda3/envs/junifer/lib/python3.9/site-packages/click/core.py", line 1055, in main rv = self.invoke(ctx) File "/home/fraimondo/anaconda3/envs/junifer/lib/python3.9/site-packages/click/core.py", line 1657, in invoke return _process_result(sub_ctx.command.invoke(sub_ctx)) File "/home/fraimondo/anaconda3/envs/junifer/lib/python3.9/site-packages/click/core.py", line 1404, in invoke return ctx.invoke(self.callback, **ctx.params) File "/home/fraimondo/anaconda3/envs/junifer/lib/python3.9/site-packages/click/core.py", line 760, in invoke return __callback(*args, **kwargs) File "/home/fraimondo/dev/tbox/junifer/junifer/api/cli.py", line 192, in collect api_collect(storage=storage) File "/home/fraimondo/dev/tbox/junifer/junifer/api/functions.py", line 199, in collect storage_object.collect() File "/home/fraimondo/dev/tbox/junifer/junifer/storage/hdf5.py", line 862, in collect file_ = element_files[i] IndexError: list index out of range ``` Closer inspection at the issue, it seems that the logic of chunking the collect is not correct: https://github.com/juaml/junifer/blob/85b5ff538d453f994562c2179c6361d5c5c79bbc/junifer/storage/hdf5.py#L858-L874 The problem is that the number of files is not multiple of the chunk_size, so the last chunk is not complete. This raises an error were `i` is 4214. And, since we are fixing this logic, the nested progress bars are not correct. Indeed, this is how it looks: ``` file-data: 100%|█████████████████████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00, 6.24it/s] file-data: 100%|█████████████████████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00, 6.13it/s] file-data: 100%|█████████████████████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00, 6.18it/s] file-data: 100%|█████████████████████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00, 6.23it/s] file-data: 100%|█████████████████████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00, 6.17it/s] file-data: 100%|█████████████████████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00, 6.20it/s] file-data: 100%|█████████████████████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00, 6.09it/s] file-data: 100%|█████████████████████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00, 6.19it/s] file-data: 100%|█████████████████████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00, 6.16it/s] file-data: 100%|█████████████████████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00, 6.08it/s] chunk: 28it [08:00, 17.17s/it]███████████████████████████████████████████████████████████████████| 100/100 [00:16<00:00, 6.24it/s] feature: 0%| ``` This is because for the "chunk" progress bar, is not possible to know the size in an enumerate: ``` for chunk_idx, chunk_start in tqdm( enumerate(range(0, element_count, chunk_size)), desc="chunk" ): ``` I would even suggest that we do not need separate progressbars due to the chunks. Indeed what we care is to show how the files are being included. So I would only keep 2 bars, one for the feature and one for the file. ### Expected Behavior Collect to run without issues. ### Steps To Reproduce Might need access to this specific project directory. ```python from junifer.storage import HDF5FeatureStorage fname = "/data/project/SPP2041/results/fraimondo/brain_size_project/storage/HCP_epi_mask_falff/HCP_epi_mask_falff.hdf5" storage = HDF5FeatureStorage(fname, single_output=False) storage.collect() ``` ### Environment ```markdown junifer: version: 0.0.1.dev927 python: version: 3.9.16 implementation: CPython dependencies: click: 8.1.3 numpy: 1.21.2 datalad: 0.17.0+872.g8c7bc10dd pandas: 1.4.1 nibabel: 3.2.2 nilearn: 0.9.0 sqlalchemy: 1.4.32 yaml: '6.0' system: platform: Linux-4.19.0-21-amd64-x86_64-with-glibc2.28 environment: LC_CTYPE: en_US.UTF-8 PATH: /usr/lib/fsl/5.0:/home/fraimondo/anaconda3/envs/junifer/bin:/home/fraimondo/anaconda3/condabin:/home/fraimondo/.dotfiles/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/X11R6/bin:/usr/local/games:/usr/games ``` ### Relevant log output _No response_ ### Anything else? _No response_
github-actions[bot] commented 2023-03-20 20:08:47 +00:00 (Migrated from github.com)
Copy link
PR Preview Action v1.3.0
Preview removed because the pull request was closed.
2023-03-24 09:56 UTC
[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.3.0 :---: Preview removed because the pull request was closed. 2023-03-24 09:56 UTC <!-- Sticky Pull Request Commentpr-preview -->
codecov[bot] commented 2023-03-20 20:23:15 +00:00 (Migrated from github.com)
Copy link

Codecov Report

Merging #198 (0387c1d) into main (6d15878) will decrease coverage by 0.03%.
The diff coverage is 91.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #198      +/-   ##
==========================================
- Coverage   93.25%   93.23%   -0.03%     
==========================================
  Files          80       80              
  Lines        3351     3383      +32     
  Branches      619      627       +8     
==========================================
+ Hits         3125     3154      +29     
  Misses        151      151              
- Partials       75       78       +3
Flag Coverage Δ
junifer 93.22% <91.17%> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
junifer/storage/hdf5.py 93.17% <91.17%> (-0.38%) ⬇️
## [Codecov](https://codecov.io/gh/juaml/junifer/pull/198?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report > Merging [#198](https://codecov.io/gh/juaml/junifer/pull/198?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (0387c1d) into [main](https://codecov.io/gh/juaml/junifer/commit/6d15878788483a7274bd77a236b370ab84adcd97?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (6d15878) will **decrease** coverage by `0.03%`. > The diff coverage is `91.17%`. [![Impacted file tree graph](https://codecov.io/gh/juaml/junifer/pull/198/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://codecov.io/gh/juaml/junifer/pull/198?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #198 +/- ## ========================================== - Coverage 93.25% 93.23% -0.03% ========================================== Files 80 80 Lines 3351 3383 +32 Branches 619 627 +8 ========================================== + Hits 3125 3154 +29 Misses 151 151 - Partials 75 78 +3 ``` | Flag | Coverage Δ | | |---|---|---| | junifer | `93.22% <91.17%> (-0.03%)` | :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. | [Impacted Files](https://codecov.io/gh/juaml/junifer/pull/198?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [junifer/storage/hdf5.py](https://codecov.io/gh/juaml/junifer/pull/198?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9zdG9yYWdlL2hkZjUucHk=) | `93.17% <91.17%> (-0.38%)` | :arrow_down: |
synchon (Migrated from github.com) requested changes 2023-03-22 09:53:23 +00:00
junifer/storage/hdf5.py
@ -24,0 +41,4 @@
The data to be chunked.
kind : str
The kind of data to be chunked.
element_count : int
synchon (Migrated from github.com) commented 2023-03-22 09:41:54 +00:00
Copy link

Missing docstring here.

Missing docstring here.
junifer/storage/hdf5.py
synchon (Migrated from github.com) commented 2023-03-22 09:50:55 +00:00
Copy link

Would be a safe measure to del to_write to further reduce memory usage.

Would be a safe measure to `del to_write` to further reduce memory usage.
junifer/storage/tests/test_hdf5.py
@ -785,7 +791,91 @@ def test_store_timeseries(tmp_path: Path) -> None:
assert_array_equal(read_df.values, data)
synchon (Migrated from github.com) commented 2023-03-22 09:51:42 +00:00
Copy link

Missing type annotations.

Missing type annotations.
fraimondo (Migrated from github.com) reviewed 2023-03-24 09:24:45 +00:00
junifer/storage/hdf5.py
fraimondo (Migrated from github.com) commented 2023-03-24 09:24:44 +00:00
Copy link

Let the GC do it when it considers it necessary. For small datasets, this might increase the computation time.

Let the GC do it when it considers it necessary. For small datasets, this might increase the computation time.
synchon (Migrated from github.com) approved these changes 2023-03-24 09:31:17 +00:00
synchon (Migrated from github.com) left a comment
Copy link

Thanks for the fix!

Thanks for the fix!
Sign in to join this conversation.
Reviewers
No reviewers
synchon
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!198
No description provided.