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

BF: set annex.merge-annex-branches=false for invocations not requiring updated remote information #3164

Merged
merged 8 commits into from Feb 15, 2019

Conversation

Projects
None yet
3 participants
@yarikoptic
Copy link
Member

yarikoptic commented Feb 14, 2019

git-annex will try to merge remote's git-annex branches pretty much for any command which could potentially benefit from updated availability information present in those updated remote git-annex
branches. But if that fails, the whole call fails, even if what we are asking does not require updated remote information. Original whining/analysis+response from @joeyh is a year old and is at https://git-annex.branchable.com/bugs/impossible_to_perform___34__read-only__34___git_annex_info_without_write_permissions/

Use cases where we ran into it already:

  • datalad ls -L of a dataset owned by someone else
  • datalad get files whenever files load is already there but partition
    is not writeable (singularity image, see poldracklab/fmriprep#1500 (comment)

So with this change we set -c annex.merge-annex-branches=false for those
git annex invocations where (I think, you check!) we do not really need any git-annex
branch being merged since we care only about local stuff.

This commit comes without a test since I have failed to simulate the
failing scenario without sudo chown someonelse -R testrepo . We could
probably do that on Travis, but I thought first to check if there
could be better ideas . If not, and tests pass - I guess we better merge soonish.

@@ -1023,6 +1023,7 @@ def _run_annex_command(self, annex_cmd,
git_options=None, annex_options=None,
backend=None, jobs=None,
files=None,
merge_annex_branches=False,

This comment has been minimized.

@yarikoptic

yarikoptic Feb 14, 2019

Author Member

oh boy -- that should have been True (I was messing around with name...)

@yarikoptic yarikoptic force-pushed the yarikoptic:bf-ro-operations branch from 887b31b to addc0b3 Feb 14, 2019

BF: set annex.merge-annex-branches=false for invocations not requirin…
…g updated remote information

pretty much for any command which could potentially benefit
from updated availability information present in updated remote git-annex
branches git-annex will try to merge those first.  But if that fails, the whole call
fails, even if what we are asking does not require updated remote information.

Use cases where we ran into it already:
-  datalad ls -L  of a dataset owned by someone else
-  datalad get files   whenever files load is already there but partition
      is not writeable (singularity image, see poldracklab/fmriprep#1500 (comment)

So with this change we set -c annex.merge-annex-branches=false for those
git annex invocations where I think we do not really need any git-annex
branch being merged since we care only about local stuff.

This commit comes without a test since I have failed to simulate the
failing scenario without sudo chown someonelse -R testrepo . We could
probably do that on Travis, but I thought first to check if there
could be better ideas

@yarikoptic yarikoptic force-pushed the yarikoptic:bf-ro-operations branch from addc0b3 to 21e6504 Feb 14, 2019

@effigies

This comment has been minimized.

Copy link

effigies commented Feb 14, 2019

without sudo chown someonelse -R testrepo

chmod -R a-w testrepo?

@yarikoptic

This comment has been minimized.

Copy link
Member Author

yarikoptic commented Feb 14, 2019

without sudo chown someonelse -R testrepo

chmod -R a-w testrepo?

my git/annex smokes something stronger -- couldn't scare it away this way ;-) can you?

BF: revert change for repo_info about not merging remote git-annex
might need a merge to discover remote uuid description
@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 14, 2019

Codecov Report

Merging #3164 into master will increase coverage by <.01%.
The diff coverage is 94.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3164      +/-   ##
==========================================
+ Coverage   90.71%   90.71%   +<.01%     
==========================================
  Files         249      249              
  Lines       32723    32757      +34     
==========================================
+ Hits        29684    29717      +33     
- Misses       3039     3040       +1
Impacted Files Coverage Δ
datalad/support/exceptions.py 80.26% <100%> (+0.13%) ⬆️
datalad/interface/ls.py 69.55% <100%> (+0.83%) ⬆️
datalad/support/annexrepo.py 88.3% <100%> (+0.01%) ⬆️
datalad/support/tests/test_annexrepo.py 96.34% <93.1%> (-0.14%) ⬇️
datalad/utils.py 86.51% <0%> (-0.09%) ⬇️

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 81bfe17...2c044e6. Read the comment docs.

@effigies

This comment has been minimized.

Copy link

effigies commented Feb 14, 2019

sudo mount --bind -o ro testrepo testrepo_ro?

Edit: It looks like this is going to depend on kernel version and libmount version. See Unix StackExchange#128366.

@yarikoptic

This comment has been minimized.

Copy link
Member Author

yarikoptic commented Feb 14, 2019

If sudo is there and can be ran, I can just chown root the directory

@yarikoptic

This comment has been minimized.

Copy link
Member Author

yarikoptic commented Feb 14, 2019

Could probably done via basic fuse layer... Also too involved

@kyleam
Copy link
Member

kyleam left a comment

I have failed to simulate the
failing scenario without sudo chown someonelse -R testrepo . We could
probably do that on Travis, but I thought first to check if there
could be better ideas

No better ideas here.

json_records = list(self._run_annex_command_json('info', opts=options))
# note - shouldn't use merge_annex_branches=False because updated
# info/description about remote UUIDs might be needed
json_records = list(self._run_annex_command_json(

This comment has been minimized.

@kyleam

kyleam Feb 14, 2019

Member

OK, but this does mean that the datalad ls -L usecase isn't resolved by this PR, right?

This comment has been minimized.

@yarikoptic

yarikoptic Feb 15, 2019

Author Member

I thought it was even after my fix but I could be wrong... The test will show. Thanks for catching this!

yarikoptic added some commits Feb 15, 2019

BF(unicode): use assure_unicode while formatting an exception
Otherwise was getting

======================================================================
ERROR: datalad.support.tests.test_annexrepo.test_ro_operations
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/nose/case.py", line 133, in run
    self.runTest(result)
  File "/usr/lib/python2.7/dist-packages/nose/case.py", line 151, in runTest
    test(result)
  File "/usr/lib/python2.7/unittest/case.py", line 393, in __call__
    return self.run(*args, **kwds)
  File "/usr/lib/python2.7/unittest/case.py", line 353, in run
    result.addError(self, sys.exc_info())
  File "/usr/lib/python2.7/dist-packages/nose/proxy.py", line 128, in addError
    formatted = plugins.formatError(self.test, err)
  File "/usr/lib/python2.7/dist-packages/nose/plugins/manager.py", line 99, in __call__
    return self.call(*arg, **kw)
  File "/usr/lib/python2.7/dist-packages/nose/plugins/manager.py", line 141, in chain
    result = meth(*arg, **kw)
  File "/usr/lib/python2.7/dist-packages/nose/plugins/logcapture.py", line 237, in formatError
    return (ec, self.addCaptureToErr(ev, records), tb)
  File "/usr/lib/python2.7/dist-packages/nose/plugins/logcapture.py", line 244, in addCaptureToErr
    records + \
  File "/usr/lib/python2.7/dist-packages/nose/util.py", line 652, in safe_str
    return str(val)
  File "/home/yoh/proj/datalad/datalad-master/datalad/support/exceptions.py", line 34, in __str__
    to_str += "\n%s" % self.msg
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 159: ordinal not in range(128)

while running the test which was supposed to error out with

datalad.support.exceptions.CommandError: CommandError: command 'sudo -n chmod -R 47521 /home/yoh/.tmp/datalad_temp_tree_test_ro_operationsvxxkb4f6/clone' failed with exitcode 1
Failed to run 'sudo -n chmod -R 47521 /home/yoh/.tmp/datalad_temp_tree_test_ro_operationsvxxkb4f6/clone' under None. Exit code=1. out= err=chmod: invalid mode: ‘47521’
Try 'chmod --help' for more information.
@yarikoptic

This comment has been minimized.

Copy link
Member Author

yarikoptic commented Feb 15, 2019

you were correct about repo_info @kyleam -- I will never trust myself again (until I do prepare a test for the change ;)). Now comes with a test via sudo - let's see if it doesn't get skipped on Travis. Works locally as well, as long as I pre-seed sudo session ;-) evil evil evil - but I found no other way

@yarikoptic

This comment has been minimized.

Copy link
Member Author

yarikoptic commented Feb 15, 2019

ha -- current failure is interesting in that it is a regression in git-annex while working in direct mode -- absent with 7.20181205+git27-g21eaaac6e-1~ndall+1 but present with current 7.20190129+git78-g3fa6be1fe-1~ndall+1 - git pull freaks out. (see below @joeyh if of interest). Since we are to abandon direct mode, I guess I will not care and just replace that part of the test (it is not actually that much relevant there)

   File "/home/travis/build/datalad/datalad/datalad/support/tests/test_annexrepo.py", line 2621, in test_ro_operations
    repo2.pull()
  File "/home/travis/build/datalad/datalad/datalad/support/gitrepo.py", line 1996, in pull
    **kwargs
  File "/home/travis/build/datalad/datalad/datalad/support/gitrepo.py", line 1956, in _call_gitpy_with_progress
    ret = callable(**git_kwargs)
  File "/home/travis/virtualenv/python2.7.14/lib/python2.7/site-packages/git/remote.py", line 808, in pull
    res = self._get_fetch_info_from_stderr(proc, progress)
  File "/home/travis/virtualenv/python2.7.14/lib/python2.7/site-packages/git/remote.py", line 675, in _get_fetch_info_from_stderr
    proc.wait(stderr=stderr_text)
  File "/home/travis/virtualenv/python2.7.14/lib/python2.7/site-packages/git/cmd.py", line 415, in wait
    raise GitCommandError(self.args, status, errstr)
GitCommandError: Cmd('/usr/lib/git-annex.linux/git') failed due to: exit code(1)
  cmdline: /usr/lib/git-annex.linux/git -c receive.autogc=0 -c gc.auto=0 -c core.bare=False --work-tree=. pull --progress -v origin refs/heads/master
  stderr: 'fatal: Couldn't find remote ref refs/heads/master
fatal: the remote end hung up unexpectedly'
BF(TST): replace not relevant trailing .pull test with .repo_info
Interesting that it is a regression in git-annex 7.20190129+git78-g3fa6be1fe-1~ndall+1
in direct mode -- worked before.
See #3164 (comment) for more info
@yarikoptic

This comment has been minimized.

Copy link
Member Author

yarikoptic commented Feb 15, 2019

didn't we all miss our lovely buildbots? ;-)

@kyleam

kyleam approved these changes Feb 15, 2019

@yarikoptic

This comment has been minimized.

Copy link
Member Author

yarikoptic commented Feb 15, 2019

Heh... NFS causes troubles obviously - will need to either skip of fails to chown or adjust NFS Mont options to allow passing root

BF/RF(TST): skip test if actual sudo chown call fails
Happens on NFS due to care being taken about root privileges etc.
For now - just skip the test if cannot chown
@yarikoptic

This comment has been minimized.

Copy link
Member Author

yarikoptic commented Feb 15, 2019

finally travis is green! and I've checked -- it does run the test on travis:

datalad.support.tests.test_annexrepo.test_ro_operations ... /tmp/datalad_temp_tree_test_ro_operationsdvy538wc/clone   [annex]  master  ✗ 2019-02-15/17:45:30  ✓  5 Bytes/5 Bytes
ok

so all good -- merging

@yarikoptic yarikoptic merged commit 4eff996 into datalad:master Feb 15, 2019

2 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
datalad-pr-dl-osx-64 DEV build started.
Details
WIP ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yarikoptic yarikoptic added this to the Release 0.11.3 milestone Feb 15, 2019

@yarikoptic yarikoptic deleted the yarikoptic:bf-ro-operations branch Feb 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.