juaml/junifer

Watch

Fork 0

[BUG]: HDF5FeatureStorage raises errors under normal operation #196

Merged

synchon merged 8 commits from update/hdf5-file-and-group-check into main

2023-03-20 16:22:23 +00:00

synchon commented

2023-03-20 14:33:23 +00:00

(Migrated from github.com)

Is there an existing issue for this?

I have searched the existing issues

Current Behavior

When using an HDF5FeatureStorage, the output log contains ERRORs related to HDF% storage.

2023-03-16 22:09:43,370 - JUNIFER - ERROR - HDF5 file not found at: /data/project/SPP2041/results/fraimondo/brain_size_project/storage/HCP_epi_mask_falff/element_100307_REST1_LR_HCP_epi_mask_falff.hdf5
2023-03-16 22:09:43,374 - JUNIFER - ERROR - `17194dc0bc12a30565cb1c46e04e2ea9` not found in: /data/project/SPP2041/results/fraimondo/brain_size_project/storage/HCP_epi_mask_falff/element_100307_REST1_LR_HCP_epi_mask_falff.hdf5

This is because the current implementation of H5io does not allow to test if a file contains certain variables. So all the "flow" of checking if the meta or certain md5 variable exists is done by try/catch blocks.

This is a not a good programming practice. For example, an IOError might be due to a failure in the underlying storage, but we will consider it as something "normal because there is no meta variable".

Solution: Implement a function in h5io to test for variables and use it.

def has_hdf5(fname, title="h5io"):
    h5py = _check_h5py()
    if isinstance(fname, str):
        if not op.isfile(fname):
            raise IOError('file "%s" not found' % fname)
    elif isinstance(fname, h5py.File):
        if fname.mode == 'w':
            raise UnsupportedOperation(
                'file must not be opened be opened with "w"'
            )
        print(fname.mode)
    else:
        raise ValueError(f'fname must be str or h5py.File, got {type(fname)}')
    if not isinstance(title, str):
        raise ValueError('title must be a string')
    if isinstance(fname, h5py.File):
        return title in fname
    else:
        with h5py.File(fname, mode='r') as fid:
            return title in fname

Expected Behavior

No errors in the log, no expceptions being raised.

Steps To Reproduce

Install junifer
Run one example with HDF5FeatureStorage

Environment

(junifer)juseless ➜  logs git:(main) ✗ junifer wtf 
junifer:
  version: 0.0.1.dev927
python:
  version: 3.9.16
  implementation: CPython
dependencies:
  click: 8.1.3
  numpy: 1.21.2
  datalad: 0.18.2
  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 When using an HDF5FeatureStorage, the output log contains ERRORs related to HDF% storage. ``` 2023-03-16 22:09:43,370 - JUNIFER - ERROR - HDF5 file not found at: /data/project/SPP2041/results/fraimondo/brain_size_project/storage/HCP_epi_mask_falff/element_100307_REST1_LR_HCP_epi_mask_falff.hdf5 2023-03-16 22:09:43,374 - JUNIFER - ERROR - `17194dc0bc12a30565cb1c46e04e2ea9` not found in: /data/project/SPP2041/results/fraimondo/brain_size_project/storage/HCP_epi_mask_falff/element_100307_REST1_LR_HCP_epi_mask_falff.hdf5 ``` This is because the current implementation of H5io does not allow to test if a file contains certain variables. So all the "flow" of checking if the `meta` or certain md5 variable exists is done by try/catch blocks. This is a not a good programming practice. For example, an IOError might be due to a failure in the underlying storage, but we will consider it as something "normal because there is no meta variable". Solution: Implement a function in h5io to test for variables and use it. ```python def has_hdf5(fname, title="h5io"): h5py = _check_h5py() if isinstance(fname, str): if not op.isfile(fname): raise IOError('file "%s" not found' % fname) elif isinstance(fname, h5py.File): if fname.mode == 'w': raise UnsupportedOperation( 'file must not be opened be opened with "w"' ) print(fname.mode) else: raise ValueError(f'fname must be str or h5py.File, got {type(fname)}') if not isinstance(title, str): raise ValueError('title must be a string') if isinstance(fname, h5py.File): return title in fname else: with h5py.File(fname, mode='r') as fid: return title in fname ``` ### Expected Behavior No errors in the log, no expceptions being raised. ### Steps To Reproduce 1. Install junifer 2. Run one example with HDF5FeatureStorage ### Environment ```markdown (junifer)juseless ➜ logs git:(main) ✗ junifer wtf junifer: version: 0.0.1.dev927 python: version: 3.9.16 implementation: CPython dependencies: click: 8.1.3 numpy: 1.21.2 datalad: 0.18.2 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_

synchon commented

2023-03-20 14:36:35 +00:00

(Migrated from github.com)

Will add to latest.inc after #199 is merged so as to not have conflicts.

Will add to `latest.inc` after #199 is merged so as to not have conflicts.

github-actions[bot] commented

2023-03-20 14:38:42 +00:00

(Migrated from github.com)

PR Preview Action v1.3.0
Preview removed because the pull request was closed.
2023-03-20 16:26 UTC

[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.3.0 :---: Preview removed because the pull request was closed. 2023-03-20 16:26 UTC

fraimondo (Migrated from github.com) reviewed

2023-03-20 14:49:02 +00:00

codecov[bot] commented

2023-03-20 14:59:15 +00:00

(Migrated from github.com)

Codecov Report

Merging #196 (b6bf310) into main (05d3ad5) will decrease coverage by 0.16%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #196      +/-   ##
==========================================
- Coverage   93.41%   93.25%   -0.16%     
==========================================
  Files          80       80              
  Lines        3355     3351       -4     
  Branches      615      619       +4     
==========================================
- Hits         3134     3125       -9     
- Misses        148      151       +3     
- Partials       73       75       +2

Flag	Coverage Δ
docs	`100.00% <ø> (ø)`
junifer	`93.24% <100.00%> (-0.16%)`	⬇️

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

Impacted Files	Coverage Δ
junifer/storage/hdf5.py	`93.54% <100.00%> (-2.38%)`	⬇️

## [Codecov](https://codecov.io/gh/juaml/junifer/pull/196?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report > Merging [#196](https://codecov.io/gh/juaml/junifer/pull/196?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (b6bf310) into [main](https://codecov.io/gh/juaml/junifer/commit/05d3ad550dfbf8ed9b6ff1da72eb855613d85ead?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (05d3ad5) will **decrease** coverage by `0.16%`. > The diff coverage is `100.00%`. [![Impacted file tree graph](https://codecov.io/gh/juaml/junifer/pull/196/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/196?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #196 +/- ## ========================================== - Coverage 93.41% 93.25% -0.16% ========================================== Files 80 80 Lines 3355 3351 -4 Branches 615 619 +4 ========================================== - Hits 3134 3125 -9 - Misses 148 151 +3 - Partials 73 75 +2 ``` | Flag | Coverage Δ | | |---|---|---| | docs | `100.00% <ø> (ø)` | | | junifer | `93.24% <100.00%> (-0.16%)` | :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/196?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/196?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9zdG9yYWdlL2hkZjUucHk=) | `93.54% <100.00%> (-2.38%)` | :arrow_down: |

fraimondo commented

2023-03-20 15:01:04 +00:00

(Migrated from github.com)

two more small tests will give us better coverage, @synchon can you do them?

synchon commented

2023-03-20 15:02:35 +00:00

(Migrated from github.com)

two more small tests will give us better coverage, @synchon can you do them?

Yeah I am adding that. I intentionally removed the tests to see where it affects.

> two more small tests will give us better coverage, @synchon can you do them? Yeah I am adding that. I intentionally removed the tests to see where it affects.

fraimondo (Migrated from github.com) approved these changes

2023-03-20 16:03:57 +00:00