[BUG] DataladDatagrabber will remove a dataset which was cloned outside Junifer.

bpoldrack commented

2022-09-13 10:26:15 +00:00

(Migrated from github.com)

Looking into the implementation, I see that on __exit__ remove is called. This is running datalad-remove without any other parameter than recursive=True.

remove has a lot of options, though, mostly to disable safeguards. For that, there's reckless which allows for different modes possibly
- ignoring that there are unsaved modifications
- ignoring that there's content in the dataset that appears to not have been pushed elsewhere
- ignoring that this location is potential "zombie" in git-annex' availability records
- ...

Since this appears to be a clean up, one would probably want to wipe out the dataset no matter what, suggesting to add reckless='kill'. However, this should be a conscious decision. Note, that not only would this prevent (needless) failure during clean up, but also speed things up.

Looking into the implementation, I see that on `__exit__` `remove` is called. This is running datalad-remove without any other parameter than `recursive=True`. `remove` has a lot of options, though, mostly to disable safeguards. For that, there's `reckless` which allows for different modes possibly - ignoring that there are unsaved modifications - ignoring that there's content in the dataset that appears to not have been pushed elsewhere - ignoring that this location is potential "zombie" in git-annex' availability records - ... Since this appears to be a clean up, one would probably want to wipe out the dataset no matter what, suggesting to add `reckless='kill'`. However, this should be a conscious decision. Note, that not only would this prevent (needless) failure during clean up, but also speed things up.

fraimondo commented

2022-10-27 10:11:01 +00:00

(Migrated from github.com)

I kept thinking about this. Mainly the issues is that the datalad dataset might exist already (if the user points to an existing clone as the datadir).

If this is the case, then we will call remove on a dataset that we did not clone. If we even add reckless='kill' we might even make a mess with this dataset.

What about these options:
1- If dataset does not exist: clone and remove with reckless='kill'.
2- If dataset exists: no clone, no remove, just drop files that we called "get" on.

@bpoldrack any suggestion?

Is there a way of checking if a dataset is already cloned?

I kept thinking about this. Mainly the issues is that the datalad dataset might exist already (if the user points to an existing clone as the `datadir`). If this is the case, then we will call `remove` on a dataset that we did not clone. If we even add `reckless='kill'` we might even make a mess with this dataset. What about these options: 1- If dataset does not exist: clone and remove with `reckless='kill'`. 2- If dataset exists: no clone, no remove, just drop files that we called "get" on. @bpoldrack any suggestion? Is there a way of checking if a dataset is already cloned?

bpoldrack commented

2022-10-27 11:00:55 +00:00

(Migrated from github.com)

@fraimondo: That's a good point.

What about these options:
1- If dataset does not exist: clone and remove with reckless='kill'.
2- If dataset exists: no clone, no remove, just drop files that we called "get" on.

Agree. Would require to keep track of what you used get for obviously. Keep in mind, that calling get on it is insufficient to determine whether you should drop afterwards, b/c it may have been there already (which get would be fine with). However, all datalad commands return a result dictionary. So you can distinguish whether get actually did anything by checking the status key in that dict (should be 'ok' as opposed to 'notneeded' for a "no-op get").

Is there a way of checking if a dataset is already cloned?

A Dataset can be instantiated with a path that does not yet exist or does not contain a dataset (yet). So, you can check Dataset(datadir).is_installed() to figure whether there already is a dataset at this location.

Furthermore, in the case of an existing dataset, you could check Dataset.id to match the expectation. This is the committed by datalad ID, that is therefore persistent across versions and locations.

@fraimondo: That's a good point. > What about these options: > 1- If dataset does not exist: clone and remove with reckless='kill'. > 2- If dataset exists: no clone, no remove, just drop files that we called "get" on. Agree. Would require to keep track of what you used `get` for obviously. Keep in mind, that calling `get` on it is insufficient to determine whether you should drop afterwards, b/c it may have been there already (which `get` would be fine with). However, all datalad commands return a result dictionary. So you can distinguish whether `get` actually did anything by checking the `status` key in that dict (should be `'ok'` as opposed to `'notneeded'` for a "no-op get"). > Is there a way of checking if a dataset is already cloned? A `Dataset` can be instantiated with a path that does not yet exist or does not contain a dataset (yet). So, you can check `Dataset(datadir).is_installed()` to figure whether there already is a dataset at this location. Furthermore, in the case of an existing dataset, you could check `Dataset.id` to match the expectation. This is the committed by datalad ID, that is therefore persistent across versions and locations.

fraimondo commented

2022-11-08 10:41:50 +00:00

(Migrated from github.com)

@bpoldrack: what do you think about this from #123 ?

Short summary:

1- On install, we check if the dataset needs to be cloned or not. If it has to be cloned, no changes. But if it is already present, we check that it was cloned from the same place (siblings URI). If more than one remote sibling is present, the we mark the dataset as "dirty". This is a flag that will be stored in the metadata of the feature, so we know that it might not have been the original datalad dataset as in the commit id.

github.com/juaml/junifer@29a290a0b0/junifer/datagrabber/datalad_base.py (L147-L183)

2- On get, we check if the file was actually downloaded or not. If it was, save it in a list to drop it on cleanup. Otherwise, set the dirty flag as true.
github.com/juaml/junifer@29a290a0b0/junifer/datagrabber/datalad_base.py (L93-L145)

3- On cleanup, just undo exactly what we did:

github.com/juaml/junifer@29a290a0b0/junifer/datagrabber/datalad_base.py (L185-L194)

@bpoldrack: what do you think about this from #123 ? Short summary: 1- On install, we check if the dataset needs to be cloned or not. If it has to be cloned, no changes. But if it is already present, we check that it was cloned from the same place (siblings URI). If more than one remote sibling is present, the we mark the dataset as "dirty". This is a flag that will be stored in the metadata of the feature, so we know that it might not have been the original datalad dataset as in the commit id. https://github.com/juaml/junifer/blob/29a290a0b03bd8bf37712d00acfadba7fc68f8e6/junifer/datagrabber/datalad_base.py#L147-L183 2- On get, we check if the file was actually downloaded or not. If it was, save it in a list to drop it on cleanup. Otherwise, set the dirty flag as true. https://github.com/juaml/junifer/blob/29a290a0b03bd8bf37712d00acfadba7fc68f8e6/junifer/datagrabber/datalad_base.py#L93-L145 3- On cleanup, just undo exactly what we did: https://github.com/juaml/junifer/blob/29a290a0b03bd8bf37712d00acfadba7fc68f8e6/junifer/datagrabber/datalad_base.py#L185-L194

bpoldrack commented

2022-11-09 09:02:39 +00:00

(Migrated from github.com)

@fraimondo:

Overall, looks good to me. I would avoid to call the flag "dirty", because in git context this term is used to indicate a worktree that is not reported "clean" by git. That may lead to confusion sooner or later.

But if it is already present, we check that it was cloned from the same place (siblings URI). If more than one remote sibling is present, the we mark the dataset as "dirty".

Pragmatically a sane detection of something that wasn't straight up cloned from the same place. However, I wonder where the original URI comes from. Why is not more specific to begin with? If you know the dataset ID and/or the desired commit the check could be more specific. Related: I'd record the dataset ID as well as the commit in the metadata. If one wants to inspect those records later, the commit alone only gives you something to detect a difference. Ultimately that depends a bit on what are the things to search for a previously used dataset, but it could be easier to find the dataset if you had the dataset ID as well.

When getting the files, you have a separate get call per file. This comes with an overhead. If you assemble a list of paths to get and pass the list to get instead, things could be faster (and possibly be done in parallel within get, if you pass jobs parameter).

Additional hint: all datalad commands take a bunch of general options. You may want to consider giving result_renderer='disabled' to the clone and get calls to not have datalad generate output on stdout but only yield the result dicts.

@fraimondo: Overall, looks good to me. I would avoid to call the flag "dirty", because in git context this term is used to indicate a worktree that is not reported "clean" by git. That may lead to confusion sooner or later. > But if it is already present, we check that it was cloned from the same place (siblings URI). If more than one remote sibling is present, the we mark the dataset as "dirty". Pragmatically a sane detection of something that wasn't straight up cloned from the same place. However, I wonder where the original URI comes from. Why is not more specific to begin with? If you know the dataset ID and/or the desired commit the check could be more specific. Related: I'd record the dataset ID as well as the commit in the metadata. If one wants to inspect those records later, the commit alone only gives you something to detect a difference. Ultimately that depends a bit on what are the things to search for a previously used dataset, but it could be easier to find the dataset if you had the dataset ID as well. When getting the files, you have a separate `get` call per file. This comes with an overhead. If you assemble a list of paths to get and pass the list to get instead, things could be faster (and possibly be done in parallel within `get`, if you pass `jobs` parameter). Additional hint: all datalad commands take a bunch of general options. You may want to consider giving `result_renderer='disabled'` to the `clone` and `get` calls to not have datalad generate output on stdout but only yield the result dicts.

bpoldrack commented

2022-11-09 09:09:29 +00:00

(Migrated from github.com)

Noticed one more:

assert self._dataset.repo is not None # avoid errors from mypy

This is relatively expensive, because repo is a property that is evaluated whenever it's accessed and that involves file system operation. If you have tested is_installed before, this is likely unnecessary, as repo can only become None when the dataset was deleted in the meantime. It may be fine to crash in this case, since this is either a bug leading to this situation or a user interfered from outside while this is running. Either way - you probably don't want to proceed anyway.

So, if you have seen mypy failing with this, I'd catch that to fail more informative (like "dataset vanished") rather than checking upfront.

I'm not sure how often this would be called, especially with a dataset with a lot of subdatasets, so the cost may be irrelevant or substantial. That's for you to judge, I guess.

Noticed one more: > assert self._dataset.repo is not None # avoid errors from mypy This is relatively expensive, because `repo` is a property that is evaluated whenever it's accessed and that involves file system operation. If you have tested `is_installed` before, this is likely unnecessary, as `repo` can only become `None` when the dataset was deleted in the meantime. It may be fine to crash in this case, since this is either a bug leading to this situation or a user interfered from outside while this is running. Either way - you probably don't want to proceed anyway. So, if you have seen mypy failing with this, I'd catch that to fail more informative (like "dataset <path> vanished") rather than checking upfront. I'm not sure how often this would be called, especially with a dataset with a lot of subdatasets, so the cost may be irrelevant or substantial. That's for you to judge, I guess.

fraimondo commented

2022-11-09 13:53:21 +00:00

(Migrated from github.com)

@bpoldrack Thanks for this. Is quite useful!

@fraimondo:

Overall, looks good to me. I would avoid to call the flag "dirty", because in git context this term is used to indicate a worktree that is not reported "clean" by git. That may lead to confusion sooner or later.

It's kind of the idea here. And I think that getting the dirty flag from git is even better. Is there any way in which we can check if the dataset is dirty? Worst-case scenario the user does "commit" but not "push". But in this case, the commit id will be recorded and nowhere to be found.

But if it is already present, we check that it was cloned from the same place (siblings URI). If more than one remote sibling is present, the we mark the dataset as "dirty".

Pragmatically a sane detection of something that wasn't straight up cloned from the same place. However, I wonder where the original URI comes from. Why is not more specific to begin with? If you know the dataset ID and/or the desired commit the check could be more specific. Related: I'd record the dataset ID as well as the commit in the metadata. If one wants to inspect those records later, the commit alone only gives you something to detect a difference. Ultimately that depends a bit on what are the things to search for a previously used dataset, but it could be easier to find the dataset if you had the dataset ID as well.

Because this is a generic class for any datalad dataset. We can ask the user to provide the URI + ID, but it will be more of the same (as the user might have to go to the repo and check for the ID). The whole idea though is not to prevent the user from running, but rather store that information for later. So someone using the features computed will have access to this information stored in the "meta":

URI
commit ID
dirty status

When getting the files, you have a separate get call per file. This comes with an overhead. If you assemble a list of paths to get and pass the list to get instead, things could be faster (and possibly be done in parallel within get, if you pass jobs parameter).

Thanks for this one, just changed the code to refelct this.

Additional hint: all datalad commands take a bunch of general options. You may want to consider giving result_renderer='disabled' to the clone and get calls to not have datalad generate output on stdout but only yield the result dicts.

For the moment I don't mind all the output. It might be useful to debug datalad-related issues from junifer. Although I think that at some point I will link the verbosity of the junifer logger to the datalad logger. So if the user select's info or debug, all output is there. Currently datalad is info even if junifer is not. I just did it and realized that there's also stdout output. I just added this result_renderer='disabled'.

Nevertheless, I must tell you that I'm upset (:P) that the output of datalad.get does not respect the order of the input:

to_get = ['sub-01_T1w.nii.gz', 'sub-01_task-rest_bold.nii.gz']
dl_out = self._dataset.get(to_get, result_renderer='disabled')
[Path(x["path"]).name for x in dl_out]
['sub-01_task-rest_bold.nii.gz', 'sub-01_T1w.nii.gz']

@bpoldrack Thanks for this. Is quite useful! > @fraimondo: > > Overall, looks good to me. I would avoid to call the flag "dirty", because in git context this term is used to indicate a worktree that is not reported "clean" by git. That may lead to confusion sooner or later. It's kind of the idea here. And I think that getting the dirty flag from git is even better. Is there any way in which we can check if the dataset is dirty? Worst-case scenario the user does "commit" but not "push". But in this case, the commit id will be recorded and nowhere to be found. > > But if it is already present, we check that it was cloned from the same place (siblings URI). If more than one remote sibling is present, the we mark the dataset as "dirty". > > Pragmatically a sane detection of something that wasn't straight up cloned from the same place. However, I wonder where the original URI comes from. Why is not more specific to begin with? If you know the dataset ID and/or the desired commit the check could be more specific. Related: I'd record the dataset ID as well as the commit in the metadata. If one wants to inspect those records later, the commit alone only gives you something to detect a difference. Ultimately that depends a bit on what are the things to search for a previously used dataset, but it could be easier to find the dataset if you had the dataset ID as well. Because this is a generic class for any datalad dataset. We can ask the user to provide the URI + ID, but it will be more of the same (as the user might have to go to the repo and check for the ID). The whole idea though is not to prevent the user from running, but rather store that information for later. So someone using the features computed will have access to this information stored in the "meta": - URI - commit ID - dirty status > > When getting the files, you have a separate `get` call per file. This comes with an overhead. If you assemble a list of paths to get and pass the list to get instead, things could be faster (and possibly be done in parallel within `get`, if you pass `jobs` parameter). Thanks for this one, just changed the code to refelct this. > > Additional hint: all datalad commands take a bunch of general options. You may want to consider giving `result_renderer='disabled'` to the `clone` and `get` calls to not have datalad generate output on stdout but only yield the result dicts. For the moment I don't mind all the output. It might be useful to debug datalad-related issues from junifer. Although I think that at some point I will link the verbosity of the junifer logger to the datalad logger. So if the user select's info or debug, all output is there. Currently datalad is info even if junifer is not. I just did it and realized that there's also stdout output. I just added this `result_renderer='disabled'`. Nevertheless, I must tell you that I'm upset (:P) that the output of `datalad.get` does not respect the order of the input: ``` to_get = ['sub-01_T1w.nii.gz', 'sub-01_task-rest_bold.nii.gz'] dl_out = self._dataset.get(to_get, result_renderer='disabled') [Path(x["path"]).name for x in dl_out] ['sub-01_task-rest_bold.nii.gz', 'sub-01_T1w.nii.gz'] ```

bpoldrack commented

2022-11-09 14:42:00 +00:00

(Migrated from github.com)

@fraimondo

Is there any way in which we can check if the dataset is dirty?

Generally: datalad status (see help for tons of options what to report). Just like git status it would message clean. A convenient way of having a look at datalad commands' result dicts is calling it with a json_pp result renderer: datalad -f json_pp status. (Note, that the CLI's -f/--output-format is the result renderer. So, 'json_pp' or 'json' would be an alternative to disabled if you'd want structured output instead of none). You can programatically assess whether everything is clean by checking whether all results have status ok and state clean.

Worst-case scenario the user does "commit" but not "push".

This would normally be assessed by datalad as a safeguard when calling drop. See drop's help regarding the --reckless switch to turn off particular or all kinds these checks. The checks obviously do come at a runtime cost, though.

I don't think we have the check available as a stand-alone thing at the moment, though.

I just did it and realized that there's also stdout output.

Yes, logging is different. This is about what we call result rendering. Commands are generators yielding those result records. You can obviously get them directly via python and independently on that, they can be rendered. The standard renderers would write to stdout. One can choose different ones. See datalad --help --> Global options --> -f. Those values can be passed to result_renderer in the python interface.

Nevertheless, I must tell you that I'm upset (:P) that the output of datalad.get does not respect the order of the input:

Hehe. Well, all datalad commands are generators and supposed to yield those results as they are accomplished, which matters a lot at scale. That means, technically we are not even in control of the order actually executed (and therefore yielded), but git-annex is (where we pass the relevant paths to). There may be something to rectify in this regard within datalad, but: We can't keep the promise of maintaining the order anyway, because of possible internal parallelization. We'd need to reorder afterwards, which would prevent us from yielding as soon as things are completed. So, unlikely to change towards that promise.

@fraimondo > Is there any way in which we can check if the dataset is dirty? Generally: `datalad status` (see help for tons of options what to report). Just like `git status` it would message `clean`. A convenient way of having a look at datalad commands' result dicts is calling it with a `json_pp` result renderer: `datalad -f json_pp status`. (Note, that the CLI's `-f/--output-format` is the result renderer. So, `'json_pp'` or `'json'` would be an alternative to `disabled` if you'd want structured output instead of none). You can programatically assess whether everything is clean by checking whether all results have `status` `ok` and `state` `clean`. > Worst-case scenario the user does "commit" but not "push". This would normally be assessed by datalad as a safeguard when calling `drop`. See `drop`'s help regarding the `--reckless` switch to turn off particular or all kinds these checks. The checks obviously do come at a runtime cost, though. I don't think we have the check available as a stand-alone thing at the moment, though. > I just did it and realized that there's also stdout output. Yes, logging is different. This is about what we call result rendering. Commands are generators yielding those result records. You can obviously get them directly via python and independently on that, they can be rendered. The standard renderers would write to stdout. One can choose different ones. See `datalad --help` --> `Global options` --> `-f`. Those values can be passed to `result_renderer` in the python interface. > Nevertheless, I must tell you that I'm upset (:P) that the output of datalad.get does not respect the order of the input: Hehe. Well, all datalad commands are generators and supposed to yield those results as they are accomplished, which matters a lot at scale. That means, technically we are not even in control of the order actually executed (and therefore yielded), but git-annex is (where we pass the relevant paths to). There may be something to rectify in this regard within datalad, but: We can't keep the promise of maintaining the order anyway, because of possible internal parallelization. We'd need to reorder afterwards, which would prevent us from yielding as soon as things are completed. So, unlikely to change towards that promise.

fraimondo commented

2022-11-09 16:01:47 +00:00

(Migrated from github.com)

@bpoldrack

I'm now testing Dataset.status to get the "state" of each file too. Now this will result in a different "meta" for dirty and non-dirty elements. So this means that dirty subjects will be grouped into a different "feature". As the "dataset" is dirty as a whole, then everything is dirty. So now my take is to do it on install which is the method that either clones the dataset, or just verifies its integrity.

The main problem resides on a dataset that it was not cloned because it was already installed. In this case, my take is to run Dataset.status() on the whole dataset. However, this gives me one entry per file. Which could be inconvenient for very big datasets. So far it works, and can't be worse than cloning the dataset, as it only needs to check the files locally.

Here's what I do now.

If there is more than one sibling with a different URI, we consider that dataset as "dirty". The ideal option would be to inspect the dataset ID from the remote URI and see if it matches the local one. Do you now if this is possible without cloning the dataset? I know it's in the .datalad/config remote file.
Check the status. If at least one file is not clean, we consider that dataset as dirty.

github.com/juaml/junifer@97e0a9ef9f/junifer/datagrabber/datalad_base.py (L161-L176)

@bpoldrack I'm now testing `Dataset.status` to get the "state" of each file too. Now this will result in a different "meta" for dirty and non-dirty elements. So this means that dirty subjects will be grouped into a different "feature". As the "dataset" is dirty as a whole, then everything is dirty. So now my take is to do it on `install` which is the method that either clones the dataset, or just verifies its integrity. The main problem resides on a dataset that it was not cloned because it was already installed. In this case, my take is to run `Dataset.status()` on the whole dataset. However, this gives me one entry per file. Which could be inconvenient for very big datasets. So far it works, and can't be worse than cloning the dataset, as it only needs to check the files locally. Here's what I do now. 1) If there is more than one sibling with a different URI, we consider that dataset as "dirty". The ideal option would be to inspect the dataset ID from the remote URI and see if it matches the local one. Do you now if this is possible without cloning the dataset? I know it's in the `.datalad/config` remote file. 2) Check the status. If at least one file is not clean, we consider that dataset as dirty. https://github.com/juaml/junifer/blob/97e0a9ef9f3db50e09da41f522993ba1557b7e0c/junifer/datagrabber/datalad_base.py#L161-L176

fraimondo commented

2022-11-09 18:37:58 +00:00

(Migrated from github.com)

For 1):

maybe use:

git clone -n URI --depth 1
git checkout HEAD .datalad/config

This gets only the file, but will it work with any URI?

For 1): maybe use: ``` git clone -n URI --depth 1 git checkout HEAD .datalad/config ``` This gets only the file, but will it work with any URI?

bpoldrack commented

2022-11-10 11:06:34 +00:00

(Migrated from github.com)

@fraimondo

The ideal option would be to inspect the dataset ID from the remote URI and see if it matches the local one. Do you now if this is possible without cloning the dataset? I know it's in the .datalad/config remote file.

maybe use:
git clone -n URI --depth 1
git checkout HEAD .datalad/config
This gets only the file, but will it work with any URI?

It should work with any URI. But this approach requires a place to checkout this remote and to hold the clone and both the checkout (even if limited to a singe file) and the clone itself are unnecessary file system operations. I would suggest something different:

I think the existence of another sibling only is relevant if it's already fetched. If no remote branches exist for it yet, then it's nothing but a git config entry, meaning that it couldn't have any influence on the state of the dataset you are trying to validate.
If there are associated branches with this sibling, no cloning is needed to check their content - you already have it.
You can get the content of a file in a not checked out branch like this: git cat-file blob <branch-name>:<relative path within repo>. This saves you writing it to disc first (your git checkout HEAD .datalad/config).

The trouble is what remote branch to look at, really in order to generalize properly. That depends on why exactly you are concerned with different siblings. If it's about what the local dataset was cloned from then there should be a remote/<name>/HEAD to look at. Usually name will be origin, but in any case - the cloned from remote should be the only one with a HEAD as far as I'm aware. That may be a good enough validation for you.

If the concern is broader I'd check the remote's branch with the same name as the branch the local dataset is on. Technically one may even want to check configured tracking branches.

But ultimately it's really about the local state and you are looking for proxy indications of deviation from the state of the "real" URI, right?
So, why not go the other way around? Find the sibling that actually is the "right" one and figure its HEAD, get the commit and compare to local HEAD?

git fetch <remote> HEAD should give you the ref locally in FETCH_HEAD. You can then find the commit with git show FETCH_HEAD (see man page to format the output to something easily parseable).

General hint: Dataset.repo.call_git() can be used to run git commands. This is making sure those calls are using the same git as datalad and git-annex in case of several being installed. There's also some convenience for getting the output, having the call be a generator and so on.
Check those:

>git grep "def call_" -- datalad/dataset/gitrepo.py
datalad/dataset/gitrepo.py:    def call_git(self, args, files=None,
datalad/dataset/gitrepo.py:    def call_git_items_(self,
datalad/dataset/gitrepo.py:    def call_git_oneline(self, args, files=None, expect_stderr=False,
datalad/dataset/gitrepo.py:    def call_git_success(self, args, files=None, expect_stderr=False,

@fraimondo > The ideal option would be to inspect the dataset ID from the remote URI and see if it matches the local one. Do you now if this is possible without cloning the dataset? I know it's in the .datalad/config remote file. > > maybe use: > ``` >git clone -n URI --depth 1 >git checkout HEAD .datalad/config >``` >This gets only the file, but will it work with any URI? It should work with any URI. But this approach requires a place to checkout this remote and to hold the clone and both the checkout (even if limited to a singe file) and the clone itself are unnecessary file system operations. I would suggest something different: 1. I think the existence of another sibling only is relevant if it's already fetched. If no remote branches exist for it yet, then it's nothing but a git config entry, meaning that it couldn't have any influence on the state of the dataset you are trying to validate. 2. If there are associated branches with this sibling, no cloning is needed to check their content - you already have it. 3. You can get the content of a file in a not checked out branch like this: `git cat-file blob <branch-name>:<relative path within repo>`. This saves you writing it to disc first (your `git checkout HEAD .datalad/config`). The trouble is what remote branch to look at, really in order to generalize properly. That depends on why exactly you are concerned with different siblings. If it's about what the local dataset was cloned from then there should be a `remote/<name>/HEAD` to look at. Usually `name` will be origin, but in any case - the cloned from remote should be the only one with a HEAD as far as I'm aware. That may be a good enough validation for you. If the concern is broader I'd check the remote's branch with the same name as the branch the local dataset is on. Technically one may even want to check configured tracking branches. But ultimately it's really about the local state and you are looking for proxy indications of deviation from the state of the "real" URI, right? So, why not go the other way around? Find the sibling that actually is the "right" one and figure its HEAD, get the commit and compare to local HEAD? `git fetch <remote> HEAD` should give you the ref locally in `FETCH_HEAD`. You can then find the commit with `git show FETCH_HEAD` (see man page to format the output to something easily parseable). General hint: `Dataset.repo.call_git()` can be used to run git commands. This is making sure those calls are using the same git as datalad and git-annex in case of several being installed. There's also some convenience for getting the output, having the call be a generator and so on. Check those: ``` >git grep "def call_" -- datalad/dataset/gitrepo.py datalad/dataset/gitrepo.py: def call_git(self, args, files=None, datalad/dataset/gitrepo.py: def call_git_items_(self, datalad/dataset/gitrepo.py: def call_git_oneline(self, args, files=None, expect_stderr=False, datalad/dataset/gitrepo.py: def call_git_success(self, args, files=None, expect_stderr=False, ```

bpoldrack commented

2022-11-10 11:15:41 +00:00

(Migrated from github.com)

One could go even further: If you take the above suggested approach of fetching current HEAD of the correct sibling and this is not the current state of the local dataset, but the local dataset is clean - then you could also store the current branch/commit and checkout the remote HEAD, run on this and checkout the previous one on clean up. That is likely cheaper than a fresh clone.

However, that kinda depends on what you want and what the datasets look like. Recursive checkout of subdatasets is somewhat cumbersome in the general case. But in a "simple" one, that may be a relatively easy performance gain.

One could go even further: If you take the above suggested approach of fetching current HEAD of the correct sibling and this is not the current state of the local dataset, but the local dataset is clean - then you could also store the current branch/commit and checkout the remote HEAD, run on this and checkout the previous one on clean up. That is likely cheaper than a fresh clone. However, that kinda depends on what you want and what the datasets look like. Recursive checkout of subdatasets is somewhat cumbersome in the general case. But in a "simple" one, that may be a relatively easy performance gain.

bpoldrack commented

2022-11-10 11:21:47 +00:00

(Migrated from github.com)

Re 2.)

That looks alright to me.

Re 2.) That looks alright to me.

fraimondo commented

2022-11-10 11:24:58 +00:00

(Migrated from github.com)

Well, I thought it through and checking the sibling is not necessary. In the end, everything I care is to figure out from which dataset it came. So when we read two sets of different features, we can easily check if they came from the same dataset or not.

So only one check is required: datalad dataset ID. If the ID is different, then we just don't allow it to continue. This is most probably an error from the user.

Now the rest can't really be checked in runtime. Even the commit ID does not tell the whole history. One could just cherry pick one commit ID on to a different branch. What we can do though, is to set a flag that indicates if the dataset its a fresh clone or not. Ideally, users should not need to run junifer on "previously" cloned datasets. But if they need to do so, they still have the option. However, any other user "re-using" features from a dirty or previously cloned dataset should know that there's a big orange flag that you can trust as much as the "CSV with features that some master's student computed one year ago".

Well, I thought it through and checking the sibling is not necessary. In the end, everything I care is to figure out from which dataset it came. So when we read two sets of different features, we can easily check if they came from the same dataset or not. So only one check is required: datalad dataset ID. If the ID is different, then we just don't allow it to continue. This is most probably an error from the user. Now the rest can't really be checked in runtime. Even the commit ID does not tell the whole history. One could just cherry pick one commit ID on to a different branch. What we can do though, is to set a flag that indicates if the dataset its a fresh clone or not. Ideally, users should not need to run junifer on "previously" cloned datasets. But if they need to do so, they still have the option. However, any other user "re-using" features from a *dirty* or *previously cloned* dataset should know that there's a big orange flag that you can trust as much as the "CSV with features that some master's student computed one year ago".

bpoldrack commented

2022-11-10 11:35:48 +00:00

(Migrated from github.com)

One could just cherry pick one commit ID on to a different branch.

@fraimondo
Edit: Actually that's not true. (Forget what I wrote before in case you saw it - I confused myself)

You cannot have the same commit SHA but not the same history. You can have the same data, but not the same history. But this is irrelevant here. If you detect the "correct" commit, everything is alright. No matter where it comes from.

A commit consists of the committed diff, obv. but also the parent commit(s), committer, author, timestamp. Any of that changes -> new commit with new commit SHA.

> One could just cherry pick one commit ID on to a different branch. @fraimondo Edit: Actually that's not true. (Forget what I wrote before in case you saw it - I confused myself) You cannot have the same commit SHA but not the same history. You can have the same data, but not the same history. But this is irrelevant here. If you detect the "correct" commit, everything is alright. No matter where it comes from. A commit consists of the committed diff, obv. but also the parent commit(s), committer, author, timestamp. Any of that changes -> new commit with new commit SHA.

fraimondo commented

2022-11-10 11:51:16 +00:00

(Migrated from github.com)

@bpoldrack I was replying to you while reading about commits SHA and stuff. And I started to doubt myself!

As it stands right now, you can do any modification on the dataset. You can avoid the flag providing that you let junifer clone the dataset from the remote.

But in any case, you can always beat the system if you want, and we can't prevent the user from doing it.

You can create a repo anywhere, modify data, cherry pick the last commit from head, push to the repo and then tell junifer to use a "fresh clone from here". This will not be flagged, however, the URI will be saved in the meta. And this is where users that want the features need to also check where the data comes from. It's not the same if it comes from the "official datalad" datasets in jugit/gin that from /p/project/new_project/test_fmriprep_2

@bpoldrack I was replying to you while reading about commits SHA and stuff. And I started to doubt myself! As it stands right now, you can do any modification on the dataset. You can avoid the flag providing that you let junifer clone the dataset from the remote. But in any case, you can always beat the system if you want, and we can't prevent the user from doing it. You can create a repo anywhere, modify data, cherry pick the last commit from head, push to the repo and then tell junifer to use a "fresh clone from here". This will not be flagged, however, the _URI_ will be saved in the meta. And this is where users that want the features need to also check where the data comes from. It's not the same if it comes from the "official datalad" datasets in jugit/gin that from `/p/project/new_project/test_fmriprep_2`

bpoldrack commented

2022-11-10 11:54:54 +00:00

(Migrated from github.com)

You can create a repo anywhere, modify data, cherry pick the last commit from head, push to the repo and then tell junifer to use a "fresh clone from here".

Ok, then that's alright for me.

> You can create a repo anywhere, modify data, cherry pick the last commit from head, push to the repo and then tell junifer to use a "fresh clone from here". Ok, then that's alright for me.

bpoldrack commented

2022-11-10 12:10:09 +00:00

(Migrated from github.com)

@fraimondo

however, the URI will be saved in the meta. And this is where users that want the features need to also check where the data comes from.

That is the part that still confuses me, though. The URI is useful, but it bears no meaning with respect to the data. It merely is a location. But git is a distributed system. Same commit is the same thing - location doesn't matter. It's useful for provenance and to actually backtrack/reproduce/find things, but it doesn't determine what exact data the extraction ran on. So, the only reason I can see to make a canonical location special is a promise of persistence - you/we/whoever promises that later on that same commit can be found there again, whereas a student's home folder is more likely to vanish. Other than that - the location seems irrelevant even to the provenance, since again - same commit == same history. That may be important, since making sure it comes from there implies we have that commit in longterm storage, but the "validity" of the data processing is not affected at all.

Just want to get that conceptually clear.

@fraimondo > however, the URI will be saved in the meta. And this is where users that want the features need to also check where the data comes from. That is the part that still confuses me, though. The URI is useful, but it bears no meaning with respect to the data. It merely is a location. But git is a distributed system. Same commit is the same thing - location doesn't matter. It's useful for provenance and to actually backtrack/reproduce/find things, but it doesn't determine what exact data the extraction ran on. So, the only reason I can see to make a canonical location special is a promise of persistence - you/we/whoever promises that later on that same commit can be found there again, whereas a student's home folder is more likely to vanish. Other than that - the location seems irrelevant even to the provenance, since again - same commit == same history. That may be important, since making sure it comes from there implies we have that commit in longterm storage, but the "validity" of the data processing is not affected at all. Just want to get that conceptually clear.

fraimondo commented

2022-11-10 12:24:05 +00:00

(Migrated from github.com)

Yes! Got it!

So we keep the URI for reproducibility. The user should be able to get the exact configuration that was used to create the features from the meta.

If the URI changes, but you have the same commit id since it's a clone on some other place, you will still end up with two different features (as they have different meta). However, this does not prevent any user from "merging" features. The user just need to compare the commit ID instead of the URI, as the URI does not indicate anything but location (and that's why I don't check the URI anymore, but dataset ID) and store the commit in the meta.

the flag I spoke before will not be added.

Yes! Got it! So we keep the URI for reproducibility. The user should be able to get the exact configuration that was used to create the features from the meta. If the URI changes, but you have the same commit id since it's a clone on some other place, you will still end up with two different features (as they have different meta). However, this does not prevent any user from "merging" features. The user just need to compare the commit ID instead of the URI, as the URI does not indicate anything but location (and that's why I don't check the URI anymore, but dataset ID) and store the commit in the meta. the flag I spoke before will not be added.

Rows
Columns

[BUG] DataladDatagrabber will remove a dataset which was cloned outside Junifer. #53