UX: Allow create-sibling-ria to only act on existing store #6045
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bpoldrack
requested changes
Oct 6, 2021
Member
bpoldrack
left a comment
bpoldrack
left a comment
There was a problem hiding this comment.
Unless there are strong objections to my suggestion I would prefer it to be the other way around. Only the first dataset per store would need to actually create it. Hence, in most cases creation isn't needed. I would go with a --create-store flag rather than expect-store.
For maint its default should probably be true, but switch to false in master. (or not have it in maint at all)
|
|
||
| create_store(SSHRemoteIO(ssh_host) if ssh_host else LocalIO(), | ||
| io = SSHRemoteIO(ssh_host) if ssh_host else LocalIO() | ||
| if not io.exists(Path(base_path)) and expect_store: |
Member
There was a problem hiding this comment.
For consistency with how the ORA remote asses the existence of a store, a valid ria-layout-version file at that path is needed, too. Mere existence of the path itself doesn't make it a useable store. Also: base_path could be a file and would pass this test.
So, I think change condition to "io.read_file(Path(base_path) / 'ria-layout-version') doesn't raise anything".
Member
Author
There was a problem hiding this comment.
I have implemented this now. But I had to take a detour via a try/except clause, because io.read_file(Path(base_path) / 'ria-layout-version') will raise FileNotFound if there is no layout. If you know a more elegant way, I'd be all ears :-)
Member
Author
|
Thanks for the review, @bpoldrack!
Sure, also makes sense. In your opinion, what should happen in case of
I'd personally prefer to not have it in maint at all |
Codecov Report
@@ Coverage Diff @@
## master #6045 +/- ##
===========================================
- Coverage 89.83% 35.89% -53.95%
===========================================
Files 319 319
Lines 42414 42424 +10
===========================================
- Hits 38102 15226 -22876
- Misses 4312 27198 +22886
Continue to review full report at Codecov.
|
Member
|
I also see this not targeting |
Member
Continue placing a sibling in it. It's |
adswa
commented
Oct 7, 2021
Comment on lines
+422
to
+425
| log_progress( | ||
| lgr.info, 'create-sibling-ria', | ||
| 'Creating a new RIA store at %s', Path(base_path), | ||
| ) |
Member
Author
There was a problem hiding this comment.
I have a question: I couldn't figure out where this logging ends up. No log level unearthed it. Where does it go, @bpoldrack?
Member
There was a problem hiding this comment.
Depends. Into a progress bar if possible. But it may end up in a log message, if there's no progress bar for some reason. See datalad.log.log_progress
adswa
force-pushed
the
ux-5982
branch
2 times, most recently
from
3654f38 to
781b3f0
Compare
bpoldrack
approved these changes
Oct 7, 2021
Member
bpoldrack
left a comment
bpoldrack
left a comment
There was a problem hiding this comment.
Minor suggestion to make the doc string a little more clear from my POV, otherwise: LGTM!
Member
Author
|
mhh, there is a test failure on macOS and crippledFS that looks like it could be related... |
adswa
commented
Oct 7, 2021
| run(['datalad', 'clone', cloneurl + '#' + orig_super.id, 'super']) | ||
| run(['datalad', '-C', 'super', 'get', '--recursive', '.']) | ||
| run(['datalad', '-C', 'super', 'get', '--recursive', '.', | ||
| '--construct-store']) |
Member
Author
|
mhhh quite a potpourri of failures... I don't see how the PR is responsible for (all of) them
```
======================================================================
ERROR: datalad.core.distributed.tests.test_clone.test_clone_datasets_root
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/runner/hostedtoolcache/Python/3.6.15/x64/lib/python3.6/site-packages/nose/case.py", line 198, in runTest
self.test(*self.arg)
File "/Users/runner/hostedtoolcache/Python/3.6.15/x64/lib/python3.6/site-packages/datalad/tests/utils.py", line 190, in _wrap_skip_if_no_network
return func(*args, **kwargs)
File "/Users/runner/hostedtoolcache/Python/3.6.15/x64/lib/python3.6/site-packages/vcr/cassette.py", line 100, in __call__
return type(self)(self.cls, args_getter)._execute_function(function, args, kwargs)
File "/Users/runner/hostedtoolcache/Python/3.6.15/x64/lib/python3.6/site-packages/vcr/cassette.py", line 114, in _execute_function
return self._handle_function(fn=handle_function)
File "/Users/runner/hostedtoolcache/Python/3.6.15/x64/lib/python3.6/site-packages/vcr/cassette.py", line 138, in _handle_function
return fn(cassette)
File "/Users/runner/hostedtoolcache/Python/3.6.15/x64/lib/python3.6/site-packages/vcr/cassette.py", line 107, in handle_function
return function(*args, **kwargs)
File "/Users/runner/hostedtoolcache/Python/3.6.15/x64/lib/python3.6/site-packages/datalad/tests/utils.py", line 563, in _wrap_with_tree
return t(*(arg + (d,)), **kw)
File "/Users/runner/hostedtoolcache/Python/3.6.15/x64/lib/python3.6/site-packages/datalad/core/distributed/tests/test_clone.py", line 151, in test_clone_datasets_root
ds = clone("///")
File "/Users/runner/hostedtoolcache/Python/3.6.15/x64/lib/python3.6/site-packages/datalad/interface/utils.py", line 483, in eval_func
return return_func(generator_func)(*args, **kwargs)
File "/Users/runner/hostedtoolcache/Python/3.6.15/x64/lib/python3.6/site-packages/datalad/interface/utils.py", line 476, in return_func
results = list(results)
File "/Users/runner/hostedtoolcache/Python/3.6.15/x64/lib/python3.6/site-packages/datalad/interface/utils.py", line 463, in generator_func
msg="Command did not complete successfully")
datalad.support.exceptions.IncompleteResultsError: Command did not complete successfully. 1 failed:
[{'action': 'install',
'message': ('Failed to clone from any candidate source URL. Encountered '
'errors per each url were:\n'
'- %s',
'https://datasets-tests.datalad.org/\n'
" CommandError: 'git -c diff.ignoreSubmodules=none clone "
'--progress https://datasets-tests.datalad.org/ '
"/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/datalad_temp_tree_test_clone_datasets_rootfi1riio8/datasets-tests.datalad.org' "
"failed with exitcode 128 [err: 'Cloning into "
"'/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/datalad_temp_tree_test_clone_datasets_rootfi1riio8/datasets-tests.datalad.org'...\n"
"fatal: repository 'https://datasets-tests.datalad.org/' not "
"found']\n"
'- https://datasets-tests.datalad.org/.git\n'
" CommandError: 'git -c diff.ignoreSubmodules=none clone "
'--progress https://datasets-tests.datalad.org/.git '
"/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/datalad_temp_tree_test_clone_datasets_rootfi1riio8/datasets-tests.datalad.org' "
"failed with exitcode 128 [err: 'Cloning into "
"'/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/datalad_temp_tree_test_clone_datasets_rootfi1riio8/datasets-tests.datalad.org'...\n"
'Fetching objects: 5\r'
'error: Unable to get pack index '
'https://datasets-tests.datalad.org/.git/objects/pack/pack-10e4c61ee138e3fd701f7fab9e68f45b20b8cff5.idx\n'
'error: Unable to find af926ef0c359556ac1d36d71f7e173d97b893ff2 '
'under https://datasets-tests.datalad.org/.git\n'
'Fetching objects: 8, done.\n'
'Cannot obtain needed blob '
'af926ef0c359556ac1d36d71f7e173d97b893ff2\n'
'while processing commit '
'd71c855ef55032962bfe205169721c781f36ae21.\n'
"error: fetch failed.']"),
'path': '/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/datalad_temp_tree_test_clone_datasets_rootfi1riio8/datasets-tests.datalad.org',
'source_url': '///',
'status': 'error',
'type': 'dataset'}]
```
|
mih
reviewed
Oct 8, 2021
Member
mih
left a comment
mih
left a comment
There was a problem hiding this comment.
Some of the test failures (all?) are related to the certificate issue we had recently. The PR is based on a relatively old state of master, maybe a rebase improves the situation.
re naming of the new flag: we are not using "construct" anywhere so far. We use "create". Unless there is a good reason for switching terms, we better not. If the Q whether or not a create of an already existing store is a problem, I'd say that we could think about something like --new-store-ok.
Member
Author
The name |
This implements a basic solution to #5982. The issue is a feature request for the ability to only act on create-sibling-ria calls if the specified store already exists. This change adds a parameter to create-sibling-ria, '--construct-store' (False by default), which, if set, creates a new store if it doesn't exist under the specified url, and otherwise fails to create a RIA sibling, warning the user that there is no store at the specified path. The parameter is intentially not named 'create-store' because this would lead to a name conflict with the 'create_store' method of ria_utils. TMP typos
TST: Adjust other tests to the API change in create-sibling-ria
Member
FTR: not a conflict really. One thing is an option to a command, the other a function name. Plus: |
Member
Author
But parameter and function have a same name and both are used in the call method. Doesn't this add unnecessary confusion? if create_store:
create_store() |
Member
Arguably, yes. But then I'd rather rename an internal function, than making it a "burden" for public API names. However, I personally don't find it particularly confusing. Things like |
Member
Author
|
ok, noted. Just let me know how you would like me to change it. |
Member
|
|
Member
Author
sorry for asking again, just to be clear, that's a new name for the method |
Closed
mih
requested changes
Oct 9, 2021
Member
mih
left a comment
mih
left a comment
There was a problem hiding this comment.
I would vote for keeping new_store_ok and keep internal function names as-is.
The test failures suggest that some (two?) new_store_ok flags are still missing in the tests.
Co-authored-by: Michael Hanke <michael.hanke@gmail.com>
Co-authored-by: Michael Hanke <michael.hanke@gmail.com>
When using an SSH url, a non-existent store would raise a RIARemoteError or RemoteCommandFailedError instead of a FileNotFoundError. This change catches these cases, and allows the creation of new stores over SSH urls again.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements a basic solution to #5982.
The issue is a feature request for the ability to only act on
create-sibling-ria calls if the specified store already
exists. This change adds a parameter to create-sibling-ria,
'--construct-store'(False by default), which, if set, creates a newstore if it doesn't exist under the specified url, and otherwise fails to
create a RIA sibling, warning the user that there is no store at the
specified path. This is an API change to the current release, which
will create a new store if it doesn't find one.
The parameter is intentially not named
create-storebecause this wouldlead to a name conflict with the
create_storemethod ofria_utils.