[BUG]: Storage URI in yaml config does not behave as I would expect.

fraimondo commented

2023-03-30 13:31:05 +00:00

(Migrated from github.com)

Is there an existing issue for this?

I have searched the existing issues

Current Behavior

I use this yaml file:

workdir: /tmp

datagrabber:
    kind: DataladAOMICPIOP1
    tasks: "restingstate"
markers:
  - name: Schaefer100x17_FC
    kind: FunctionalConnectivityParcels
    parcellation: Schaefer100x17
    cor_method: correlation
storage: 
  kind: SQLiteFeatureStorage
  uri: ../storage/PIOP1
queue:
  jobname: TestHTCondorQueue
  kind: HTCondor
  env:
    kind: conda
    name: junifer
  mem: 8G

I would expect that the storage uri is interpreted relative from the directory where I run junifer queue, but it is interpreted relative to the cwd of the process.

Expected Behavior

I think it will be more intuitive to interpret relative from the directory where i run the junifer queue, i.e. my "perceived working directory".

Steps To Reproduce

Install junifer.

Use this yaml file and run junifer queue

workdir: /tmp

datagrabber:
    kind: DataladAOMICPIOP1
    tasks: "restingstate"
markers:
  - name: Schaefer100x17_FC
    kind: FunctionalConnectivityParcels
    parcellation: Schaefer100x17
    cor_method: correlation
storage: 
  kind: SQLiteFeatureStorage
  uri: ../storage/PIOP1
queue:
  jobname: TestHTCondorQueue
  kind: HTCondor
  env:
    kind: conda
    name: junifer
  mem: 8G

Environment

❱ junifer wtf
junifer:
  version: 0.0.1.dev909
python:
  version: 3.10.6
  implementation: CPython
dependencies:
  click: 8.1.3
  numpy: 1.22.4
  datalad: 0.17.9
  pandas: 1.4.4
  nibabel: 4.0.2
  nilearn: 0.9.2
  sqlalchemy: 1.4.44
  yaml: '6.0'
system:
  platform: Linux-4.19.0-21-amd64-x86_64-with-glibc2.28
environment:
  LC_CTYPE: en_US.UTF-8
  PATH: /home/lsasse/miniconda3/envs/junifer/bin:/home/lsasse/miniconda3/condabin:/home/lsasse/.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 I use this yaml file: ``` workdir: /tmp datagrabber: kind: DataladAOMICPIOP1 tasks: "restingstate" markers: - name: Schaefer100x17_FC kind: FunctionalConnectivityParcels parcellation: Schaefer100x17 cor_method: correlation storage: kind: SQLiteFeatureStorage uri: ../storage/PIOP1 queue: jobname: TestHTCondorQueue kind: HTCondor env: kind: conda name: junifer mem: 8G ``` I would expect that the storage uri is interpreted relative from the directory where I run `junifer queue`, but it is interpreted relative to the cwd of the process. ### Expected Behavior I think it will be more intuitive to interpret relative from the directory where i run the `junifer queue`, i.e. my "perceived working directory". ### Steps To Reproduce Install junifer. Use this yaml file and run `junifer queue` ``` workdir: /tmp datagrabber: kind: DataladAOMICPIOP1 tasks: "restingstate" markers: - name: Schaefer100x17_FC kind: FunctionalConnectivityParcels parcellation: Schaefer100x17 cor_method: correlation storage: kind: SQLiteFeatureStorage uri: ../storage/PIOP1 queue: jobname: TestHTCondorQueue kind: HTCondor env: kind: conda name: junifer mem: 8G ``` ### Environment ```markdown ❱ junifer wtf junifer: version: 0.0.1.dev909 python: version: 3.10.6 implementation: CPython dependencies: click: 8.1.3 numpy: 1.22.4 datalad: 0.17.9 pandas: 1.4.4 nibabel: 4.0.2 nilearn: 0.9.2 sqlalchemy: 1.4.44 yaml: '6.0' system: platform: Linux-4.19.0-21-amd64-x86_64-with-glibc2.28 environment: LC_CTYPE: en_US.UTF-8 PATH: /home/lsasse/miniconda3/envs/junifer/bin:/home/lsasse/miniconda3/condabin:/home/lsasse/.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_

fraimondo commented

2023-03-28 10:40:56 +00:00

(Migrated from github.com)

We just decided with @synchon that the relative paths of the storage URI should be kept relative to the location of the YAML file and not to the CWD. @LeSasse what do you think?

LeSasse commented

2023-03-28 11:09:53 +00:00

(Migrated from github.com)

Yes, i think always relative from the YAML file will work. But I partly see this is as part of a larger issue of relative paths issues, for example when registering parcellations, coordinates, masks etc. Perhaps it could be good to have a specific section in the docs addressing relative paths in different types of situation, and how junifer interprets them (so like a summary of different relative path situations, where if one doesnt quite remember about a specific situation, one can quickly go and have a look).

synchon commented

2023-03-28 12:03:36 +00:00

(Migrated from github.com)

That is true and the goal would be to have with and storage.uri blocks be relative to original YAML and explicitly put it in docs.

That is true and the goal would be to have `with` and `storage.uri` blocks be relative to original YAML and explicitly put it in docs.

👍 1

fraimondo commented

2023-03-28 12:05:32 +00:00

(Migrated from github.com)

Relatives paths are tricky. For the with section, the only one that makes sense is to have it relative to the location of the YAML file. For example, you can use the juni-farm repository as a submodule in a repo with your YAML files.

That's why we decided to keep it consistent and always use relative paths in the same way.

About parcellations and masks, you are right. We need to be cristal clear. Also, in this case, the user is creating a python file. So the user can also compute the absolute path relative to the file. I think that it will even make more sense to prevent registering a mask/parcellation that relies on a relative file. There's actually no reason for this to be available as a feature.

Relatives paths are tricky. For the `with` section, the only one that makes sense is to have it relative to the location of the YAML file. For example, you can use the juni-farm repository as a submodule in a repo with your YAML files. That's why we decided to keep it consistent and always use relative paths in the same way. About parcellations and masks, you are right. We need to be cristal clear. Also, in this case, the user is creating a python file. So the user can also compute the absolute path relative to the file. I think that it will even make more sense to prevent registering a mask/parcellation that relies on a relative file. There's actually no reason for this to be available as a feature.

LeSasse commented

2023-03-28 12:09:55 +00:00

(Migrated from github.com)

The problem with the registering is that the relation from the file to the parcellation/data ressource changes from when the user creates the python file. That is, if the user doesnt get that it will be relative from junifer's working directory (because junifer copies it over to the other directory) and supposes that it will be relative from their python file, computing the absolute path also won't work.

The problem from a user perspective is, that most will not like having to actually put absolute paths, for reproducibility and portability reasons of a pipeline (myself included). I want my paths in a project to be relative within a project directory.

The problem with the registering is that the relation from the file to the parcellation/data ressource changes from when the user creates the python file. That is, if the user doesnt get that it will be relative from junifer's working directory (because junifer copies it over to the other directory) and supposes that it will be relative from their python file, computing the absolute path also won't work. The problem from a user perspective is, that most will not like having to actually put absolute paths, for reproducibility and portability reasons of a pipeline (myself included). I want my paths in a project to be relative within a project directory.

fraimondo commented

2023-03-28 12:11:48 +00:00

(Migrated from github.com)

True. Masks/parcellations are not copied. Let's open an issue to see how we can tackle this.

fraimondo commented

2023-03-30 13:26:41 +00:00

(Migrated from github.com)

Update on this one: It is way too complicated to keep the storage URI relative in the "queue" function.

For the run function to work, we need to compute the absolute path on the YAML. This can be done after parsing the YAML.

However, for the queue function to run, we are creating a new YAML in $(CWD)/junifer_jobs/jobname. So in order to keep the path relative, we need to compute the relative relation between the CWD and the location of the yaml and add ../../.

I find this complicated and confusing.

So I'm going for the easy solution: in the yaml parser, compute the absolute path for any relative URI.

Update on this one: It is way too complicated to keep the storage URI relative in the "queue" function. For the `run` function to work, we need to compute the absolute path on the YAML. This can be done after parsing the YAML. However, for the `queue` function to run, we are creating a new YAML in `$(CWD)/junifer_jobs/jobname`. So in order to keep the path relative, we need to compute the relative relation between the CWD and the location of the yaml and add `../../`. I find this complicated and confusing. So I'm going for the easy solution: in the yaml parser, compute the absolute path for any relative URI.

codecov[bot] commented

2023-03-30 13:34:17 +00:00

(Migrated from github.com)

Codecov Report

Merging #127 (ae3f975) into main (fef6f93) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #127      +/-   ##
==========================================
+ Coverage   93.53%   93.62%   +0.08%     
==========================================
  Files          80       80              
  Lines        3452     3515      +63     
  Branches      648      658      +10     
==========================================
+ Hits         3229     3291      +62     
- Misses        145      147       +2     
+ Partials       78       77       -1

Flag	Coverage Δ
docs	`100.00% <ø> (ø)`
junifer	`93.62% <100.00%> (+0.08%)`	⬆️

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

Impacted Files	Coverage Δ
junifer/api/parser.py	`95.00% <100.00%> (+6.42%)`	⬆️

... and 4 files with indirect coverage changes

## [Codecov](https://codecov.io/gh/juaml/junifer/pull/127?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report > Merging [#127](https://codecov.io/gh/juaml/junifer/pull/127?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (ae3f975) into [main](https://codecov.io/gh/juaml/junifer/commit/fef6f93a9d02a5f3f3f43e52387decf971b524cf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (fef6f93) will **increase** coverage by `0.08%`. > The diff coverage is `100.00%`. [![Impacted file tree graph](https://codecov.io/gh/juaml/junifer/pull/127/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/127?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #127 +/- ## ========================================== + Coverage 93.53% 93.62% +0.08% ========================================== Files 80 80 Lines 3452 3515 +63 Branches 648 658 +10 ========================================== + Hits 3229 3291 +62 - Misses 145 147 +2 + Partials 78 77 -1 ``` | Flag | Coverage Δ | | |---|---|---| | docs | `100.00% <ø> (ø)` | | | junifer | `93.62% <100.00%> (+0.08%)` | :arrow_up: | 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/127?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [junifer/api/parser.py](https://codecov.io/gh/juaml/junifer/pull/127?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9hcGkvcGFyc2VyLnB5) | `95.00% <100.00%> (+6.42%)` | :arrow_up: | ... and [4 files with indirect coverage changes](https://codecov.io/gh/juaml/junifer/pull/127/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml)

github-actions[bot] commented

2023-03-30 13:36:11 +00:00

(Migrated from github.com)

PR Preview Action v1.3.0
Preview removed because the pull request was closed.
2023-03-30 19:48 UTC

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

synchon commented

2023-03-30 16:18:46 +00:00

(Migrated from github.com)

@fraimondo Coverage seems to be causing issue.

fraimondo commented

2023-03-30 16:28:17 +00:00

(Migrated from github.com)

@fraimondo Coverage seems to be causing issue.

This should be 100% now.

> @fraimondo Coverage seems to be causing issue. This should be 100% now.

Rows
Columns

[BUG]: Storage URI in yaml config does not behave as I would expect. #127

Is there an existing issue for this?

Current Behavior

Expected Behavior

Steps To Reproduce

Environment

Codecov Report