Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NF: create-sibling-ria command #4124

Merged
merged 15 commits into from Feb 19, 2020
Merged

NF: create-sibling-ria command #4124

merged 15 commits into from Feb 19, 2020

Conversation

mih
Copy link
Member

@mih mih commented Feb 8, 2020

This is the code that was used to push the HCP datasets to store.datalad.org -- written by @bpoldrack (see datalad/git-annex-ria-remote#38). It is one of the last pieces of RIA functionality to move into -core (the actual special remote implementation being the very last one). It is being proposed as a separate PR, because it can function without it (HCP doesn't use the ria-special remote), and to keep the complexity of the PR more manageable.

Feedback is very much welcome! Thanks!

Related:

Update by @bpoldrack :

From my point of view, this is ready. Above mentioned UX issues are addressed and test failures fixed (includes a fix for #4151). A long the way I doscovered #4152 and #4153. However, those are neither fixed herein nor are fixes for that actually required for this PR.

Tests currently depend on installing https://github.com/datalad/git-annex-ria-remote@transition-to-core, which removes the conflicting implementation of create-sibling-ria therein. While this seems somewhat ugly, it should be a quite short lived approach. Rational: We are moving the special remote to core. I'm currently working on PR #4068 to integrate execution via a persistent remote shell. When this is done, git-annex-ria-remote can go into core completely. If we had an actual dependency on the ria_remote package via setup.py, this would lead to people installing it, when shortly afterwards it will become at least unnecessary, if not conflicting to have it installed, which is why I consider this approach superior.

Edit by @bpoldrack :

Plans changed. Since there's some time pressure and a new idea re special remote configuration ( #4124 (comment) ), updated plan for ria-remote's move to core:

  • move ria-remote as is with remote execution factored out for easier RF later on.
  • once it's merged:
    • this PR (create-sibling-ria) can remove Travis dependency it properly reference/import within core and be merged
    • then special remote's initremote can be adapted to store the annex UUID in the bare repo's config
    • and clone then needs to be aware to make use of it

Note, that integration with #4068 is thereby postponed.

So, 3 PRs:

  • the main move to core
  • this very PR with adaptions
  • change RIARemote.initremote() and datalad-clone

@mih mih added this to the 0.12.3 milestone Feb 8, 2020
@mih
Copy link
Member Author

mih commented Feb 8, 2020

@bpoldrack can you have a look at the test failures please, it seems we are missing a few of their idiosyncracies in datalad-core (which should preferably by removed). Thx!

bpoldrack added 3 commits Feb 11, 2020
git-annex-ria-remote is about to move completely into datalad core. Temporarily install on Travis in order to not rewrite tests for a few days

Additionally, disable test on windows due to issue with short paths on some windows environments.
@codecov
Copy link

codecov bot commented Feb 11, 2020

Codecov Report

No coverage uploaded for pull request base (master@2bbcf92). Click here to learn what that means.
The diff coverage is 93.86%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4124   +/-   ##
=========================================
  Coverage          ?   89.54%           
=========================================
  Files             ?      279           
  Lines             ?    36707           
  Branches          ?        0           
=========================================
  Hits              ?    32868           
  Misses            ?     3839           
  Partials          ?        0
Impacted Files Coverage Δ
datalad/interface/common_opts.py 100% <ø> (ø)
datalad/distributed/create_sibling_gitlab.py 74.67% <ø> (ø)
datalad/core/local/tests/test_run.py 99.55% <ø> (ø)
datalad/local/tests/test_subdataset.py 100% <ø> (ø)
datalad/customremotes/tests/test_archives.py 89.54% <ø> (ø)
datalad/core/local/tests/test_diff.py 99.48% <ø> (ø)
datalad/core/local/tests/test_resulthooks.py 100% <ø> (ø)
datalad/support/tests/test_fileinfo.py 100% <ø> (ø)
datalad/core/local/tests/test_status.py 98.85% <ø> (ø)
datalad/tests/test_utils_testrepos.py 100% <ø> (ø)
... and 89 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2bbcf92...e790b9b. Read the comment docs.

@bpoldrack bpoldrack added the merge-if-ok label Feb 11, 2020
@bpoldrack bpoldrack marked this pull request as ready for review Feb 11, 2020
@bpoldrack
Copy link
Member

bpoldrack commented Feb 12, 2020

Added trust-level option. @mih: You might want to have a quick look at ff095ff

@mih
Copy link
Member Author

mih commented Feb 13, 2020

@bpoldrack That last patch tripped the tests

@bpoldrack
Copy link
Member

bpoldrack commented Feb 13, 2020

Note, that datalad/git-annex-ria-remote#44 and datalad/git-annex-ria-remote#45 are addressed here.

Re test: That's strange. unexpected keyword argument should happen locally as well if legit. But doesn't. Hmm.

to invoke a git-annex-[trust|semitrust|untrust] call to the created
special remote. Since the plain git remote is set to annex-ignore
anywway, I guess it's reasonable to do this for the special remote only.

(Closes datalad/git-annex-ria-remote#45)
@bpoldrack bpoldrack removed the merge-if-ok label Feb 13, 2020
@bpoldrack
Copy link
Member

bpoldrack commented Feb 13, 2020

FTR: ATM I have no idea waht's going on on Travis. Can't reproduce and looks like it has nothing to do with my last commit.

However, meanwhile fixing an issue wrt --existing. Therefor removed merge label for now.

bpoldrack added 2 commits Feb 13, 2020
- remove 'replace' mode
- check for existing target repo upfront to avoid failing/skipping only after initremote/enableremote
- remove/change outdated comments
We need the actual special remote for testing, but master branch has a conflicting create-sibling-ria command installed.
@bpoldrack bpoldrack force-pushed the nf-riasibling branch 3 times, most recently from 0ade207 to 80712bb Compare Feb 14, 2020
@bpoldrack
Copy link
Member

bpoldrack commented Feb 14, 2020

Finally able to reproduce issue on Travis. Reason: Happens only on the merge commit with master. At least twofold:

  1. publish tries to fetch special remote #4151 (easy fix)
  2. Somehow a url...insteadOf config gets lost when cloning. No matter whether I store it global or use override with datalad.cfg. Having a breakpoint in the test, creating proper global config manually succeeds.

Edit:
Just noting it down here. May turn into a fix right here or into an issue otherwise.

As suspected (but not traced down) before: We shoot our feet with the fake $HOME we set up for tests:

(Pdb) cfg.__dict__
{'_store': {'user.name': 'Benjamin Poldrack', 'user.email': 'benjaminpoldrack@gmail.com', 'url.ria+file:///tmp/datalad_temp__test_create_storele6bc8ij.insteadof': 'ria+ssh://test-store:', 'datalad.datasets.topurl': 'http://datasets-tests.datalad.org/', 'datalad.log.level': '100'}, '_cfgfiles': {'/home/ben/.gitconfig'}, '_cfgmtimes': {'/home/ben/.gitconfig': 1581683659.070651}, '_dataset_path': None, '_dataset_cfgfname': None, '_repo_cfgfname': None, '_config_cmd': ['git', '--git-dir=', 'config'], 'overrides': {}, '_src_mode': 'any', '_runner': <datalad.cmd.GitRunner object at 0x7f86b2795520>, '_gitconfig_has_showorgin': True}

Note, that it reads /home/ben/.gitconfig.
But:

(Pdb) os.environ['HOME']
'/tmp/datalad_temp_ft_s2hz0'

Edit 2:

And number 3:
clone is buggy. Looks for RIA special remote, before annex-init, which is why it's not yet enabled and therefore not found.

Edit 3:

Above mentioned wrong order brought some headache, since it should only lead to not setting the publication dependency. Instead, special remote wasn't enabled at all. How so? Turns out, that the search for such a special remote results in annex-calls like git-annex info --json .... Problem with that? It doesn't actually run git-annex-init (no respective message and no special remote enabling), but it creates .git/annex. Later on, we check whether we need to annex-init by calling AnnexRepo.is_initialized(). And this does check for the existence of .git/annex. Bang - no init, no autoenabled special remote. Apart from doing things in the right order to begin with, this leads to the question whether this is an annex bug. If not, then I don't see a way of detecting whether it really is initialized. The respective entry in .git/config (annex.uuid) was created by git-annex-info. So, looks like an annex bug to me, that annex-init is only partially executed in that case.

bpoldrack added 4 commits Feb 14, 2020
Before we can deal with special remotes, we need to figure it's an annex
and initialize it.
Also: Local config settings in postprocessing don't make sense, if we
only afterwards double-check that we do indeed have an installed dataset
as a result, etc.
Furthermore, there's a possible bug in annex, where git-annex-info
(indirectly called by postclonecfg_ria) half-initializes the repo
preventing us from later detect, that we still need to do it
(particularly in order to autoenable special remotes).
See issue datalad#4152.
@bpoldrack bpoldrack added the merge-if-ok label Feb 14, 2020
@mih
Copy link
Member Author

mih commented Feb 18, 2020

I have lost track of all the moving pieces here. Could you please leave a note in the top comment on the status of this PR. It looks ready function-wise, but there is still the test dependency issue on that branch in ria-remote. What is the general plan?

@bpoldrack
Copy link
Member

bpoldrack commented Feb 18, 2020

@mih: Updated top-level comment.

@mih
Copy link
Member Author

mih commented Feb 18, 2020

Idea:

  • when create-sibling-ria has call initremote we record the UUID of the special remote in the config of the bare repo in the store
  • when we datalad clone from a store and the special remote is not functional, we check that store config, and enableremote the special remote with the matching UUID to use the store URL and access protocol that has just led to a successful clone

This avoids the need for any url...insteadof config, and always results in an appropriate (clone-matched) access to the RIA remote.

@mih
Copy link
Member Author

mih commented Feb 18, 2020

% datalad create-sibling-ria -s datastore THISISIT
[INFO   ] create siblings 'datastore' and 'datastore-ria' ... 
[INFO   ] Fetching updates for <Dataset path=/tmp/test> 
CommandError: command '['git', 'fetch', '--verbose', '--progress', '--prune', '--recurse-submodules=no', 'datastore']' failed with exitcode 128
Failed to run ['git', 'fetch', '--verbose', '--progress', '--prune', '--recurse-submodules=no', 'datastore'] at '/tmp/test'.
stdout=
stderr=fatal: unable to find remote helper for 'ria+ssh'

fatal: unable to find remote helper for 'ria+ssh'

% git config -l |grep THISIS
url.ria+ssh://mih@XXX.YYY.de/tmp/tmpstore.insteadof=THISISIT

The failure doesn't happen with a URL that doesn't have a user specified. It is likely a URL parsing issue.

Consider something like url.ria+ssh://user@host/some/where.insteadOf MYLABEL
and using it with datalad-clone or datalad-create-sibling-ria. That would
mean to pass just MYLABEL as the url parameter. We wouldn't even recognize
it as an URL before rewriting.
@bpoldrack
Copy link
Member

bpoldrack commented Feb 18, 2020

The failure doesn't happen with a URL that doesn't have a user specified. It is likely a URL parsing issue.

Addressed in b4533ea

@bpoldrack
Copy link
Member

bpoldrack commented Feb 18, 2020

As for the annex UUID in bare repo's config: Will move ria-remote to core first and only then implement it in its initremote. This leads to updated plans - see toplevel.

@bpoldrack bpoldrack removed the merge-if-ok label Feb 18, 2020
Copy link
Member

@yarikoptic yarikoptic left a comment

Minor comments on names and wording

datalad/distributed/create_sibling_ria.py Outdated Show resolved Hide resolved
datalad/distributed/create_sibling_ria.py Outdated Show resolved Hide resolved
crucial when [CMD: --shared=group CMD][PY: shared="group" PY]""",
constraints=EnsureStr() | EnsureNone()),
ria_remote=Parameter(
args=("--no-ria-remote",),
Copy link
Member

@yarikoptic yarikoptic Feb 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name is confusing given the have of the command. May be make it --no-ria-archive if that what it is?

Copy link
Member

@bpoldrack bpoldrack Feb 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential confusion - true. But it's not about an archive either. It's about whether or not to have a special remote, since it can be useful to have that ID based tree of bare repos without any annex object.

Copy link
Member Author

@mih mih Feb 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @bpoldrack

datalad/distributed/create_sibling_ria.py Outdated Show resolved Hide resolved
@bpoldrack
Copy link
Member

bpoldrack commented Feb 19, 2020

@yarikoptic : Addressed your comments (and pushed) except for the one answered. Don't see a better name for --no-ria-remote ATM.

@bpoldrack
Copy link
Member

bpoldrack commented Feb 19, 2020

FTR: Windows failure is in datalad.tests.test_utils.test_get_open_files and looks like usual path resolution issue with short paths. No idea how this would be related to this PR.

Note, that this is about github Win2019 tests. appveyor fails with the known coverage issue only.

Restarted build after failure didn't happen in master. Now it passes. So - whatever.

@mih mih merged commit c02b19b into datalad:master Feb 19, 2020
14 of 15 checks passed
@mih mih deleted the nf-riasibling branch Feb 19, 2020
@yarikoptic yarikoptic modified the milestones: 0.12.3, 0.12.x Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants