OPT: save - do not bother running full status within subdatasets unless recursive #4526
Conversation
Situation is probably abnormal, and something else would throw a proper exception, or someone else would fix it properly in some other place.
|
ok, workaround (or may be a fix?) for infinite recursion is in d5d9670. Test became faster - only 11 sec on my laptop ;) |
Codecov Report
@@ Coverage Diff @@
## master #4526 +/- ##
==========================================
+ Coverage 89.20% 89.23% +0.02%
==========================================
Files 285 285
Lines 38558 38575 +17
==========================================
+ Hits 34397 34423 +26
+ Misses 4161 4152 -9
Continue to review full report at Codecov.
|
| @@ -206,6 +206,8 @@ def __call__(path=None, message=None, dataset=None, | |||
| recursive=recursive, | |||
| recursion_limit=recursion_limit, | |||
| on_failure='ignore', | |||
| # for save without recursion only commit matters | |||
| eval_subdataset_state='full' if recursive else 'commit', | |||
There was a problem hiding this comment.
Indeed ;-) but may be he would come up even with a better one which would work for recursive with a limit?
There was a problem hiding this comment.
Because you bring up recursion limits: Have you ever seen any actual use of it? I mean outside of tests. My shell history doesn't have it. This feels like a historical artifact of "why not have a limit too". It causes quite a few complications by requiring a relatively high level of sophistication in some code pieces. I am afraid that we will add another one here.
There was a problem hiding this comment.
consciously -- I do not recollect using it. My eternal shell history setup is broken, but on smaug I found entire 2 invocations for diff:
datalad diff -r --recursion-limit 1 openneuro
datalad diff -r --recursion-limit 2 openneuro
which kinda makes sense BUT could have been worked around by -C or -d openneuro and not using -r. So indeed -- I don't have immediate use cases. Moreover I think it would make sense only in "homogeneous" hierarchy, e.g. like HCP dataset (if it had more subdataset levels ;-)) -- probably a rare use case on its own. In all others it would likely be on a single path/subdataset, thus could be workedaround.
There was a problem hiding this comment.
gy, search on github brings up https://github.com/recoveringyank/T1ify/blob/1772167b3d771632441377fc47c729cc33d82a4b/get_data_datalad.sh with
datalad install ///hcp-openaccess -r --recursion-limit 3
when I exclude some repos I still do not see more of 3rd party relevant hints arriving... so at least nobody uses it heavily in the code besides us interfacing from one function to another ;)
| sub1.save('sub2') | ||
| # and should fail if we demand recursive operation | ||
| # Fun part: causes RecursionError, not just CommandError ATM but that is IMHO | ||
| # a separate issue, TODO. |
There was a problem hiding this comment.
Isn't this comment stale as of d5d9670?
There was a problem hiding this comment.
d'oh right -- will remove. It seems that everyone agrees, so I will remove, commit, merge locally and push master to avoid needless round of CI
|
did commits cleanup, merge locally 4f0876e, and pushed |
Closes: #4523
I guess if all the other tests pass, this change does not break any "semantic" which would be great.
Notes:
saveis "non trivial" even in this tiny datasets, heh.Possible TODOs:
recursion_limitparameter, which suggests that this location is not ideal for this fix (if there is any better).Here is the displayed traceback which I do not know where to attribute to
at the bottom of this: