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

No GitPython #4172

Merged
merged 10 commits into from Mar 6, 2020
Merged

No GitPython #4172

merged 10 commits into from Mar 6, 2020

Conversation

mih
Copy link
Member

@mih mih commented Feb 20, 2020

The end of a long process. With this PR the only piece left is ok_clean_git() that still uses it (158 times), but only in the tests. We have assert_repo_status() as a replacement, but the mapping is still effortful.

DataLad 0.11.8 on time datalad create mike

datalad create mike  0.58s user 0.24s system 92% cpu 0.881 total
datalad create mike  0.61s user 0.23s system 99% cpu 0.842 total
datalad create mike  0.51s user 0.27s system 93% cpu 0.825 total

This PR on time datalad create mike

datalad create mike  0.38s user 0.20s system 99% cpu 0.582 total
datalad create mike  0.35s user 0.16s system 99% cpu 0.511 total
datalad create mike  0.39s user 0.14s system 94% cpu 0.556 total

This is a 30% boost.

Fixes #2970
Fixes #2879
Fixes #753

@mih mih added the do not merge label Feb 20, 2020
@codecov
Copy link

codecov bot commented Feb 20, 2020

Codecov Report

Merging #4172 into master will decrease coverage by 0.20%.
The diff coverage is 96.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4172      +/-   ##
==========================================
- Coverage   89.45%   89.25%   -0.21%     
==========================================
  Files         275      275              
  Lines       36328    36168     -160     
==========================================
- Hits        32498    32281     -217     
- Misses       3830     3887      +57     
Impacted Files Coverage Δ
datalad/customremotes/base.py 69.25% <0.00%> (-14.89%) ⬇️
datalad/customremotes/tests/__init__.py 91.66% <0.00%> (-8.34%) ⬇️
datalad/support/keyring_.py 84.44% <0.00%> (-6.67%) ⬇️
datalad/tests/test_base.py 96.55% <0.00%> (-3.45%) ⬇️
datalad/customremotes/tests/test_archives.py 86.27% <0.00%> (-3.27%) ⬇️
datalad/__init__.py 90.37% <0.00%> (-1.49%) ⬇️
datalad/cmd.py 88.51% <0.00%> (-1.39%) ⬇️
datalad/tests/utils_testrepos.py 92.92% <0.00%> (-0.89%) ⬇️
datalad/core/local/create.py 93.38% <0.00%> (-0.74%) ⬇️
datalad/tests/test_cmd.py 96.79% <0.00%> (-0.54%) ⬇️
... and 5 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 8a174f5...01bc301. Read the comment docs.

@yarikoptic yarikoptic added this to the 0.13.0 milestone Feb 20, 2020
@mih mih force-pushed the playme branch 2 times, most recently from 55d8b87 to e65884e Compare Feb 21, 2020
@mih mih removed the do not merge label Feb 21, 2020
@mih
Copy link
Member Author

mih commented Feb 21, 2020

Windows failures are due to unexpectedly open files (familiar from previous PRs).

Travis failure is old #3929 -- just forced to a crash by rmtree instead of being a silent sysadmin nightmare.

======================================================================
1230ERROR: datalad.support.tests.test_annexrepo.test_dropkey(True,)
...
1243 File "/home/travis/virtualenv/python3.5.6/lib/python3.5/site-packages/datalad/utils.py", line 1922, in _rmtree
1244 return shutil.rmtree(*args, **kwargs)
1245 File "/home/travis/virtualenv/python3.5.6/lib/python3.5/shutil.py", line 480, in rmtree
1246 _rmtree_safe_fd(fd, path, onerror)
1247 File "/home/travis/virtualenv/python3.5.6/lib/python3.5/shutil.py", line 418, in _rmtree_safe_fd
1248 _rmtree_safe_fd(dirfd, fullname, onerror)
1249 File "/home/travis/virtualenv/python3.5.6/lib/python3.5/shutil.py", line 418, in _rmtree_safe_fd
1250 _rmtree_safe_fd(dirfd, fullname, onerror)
1251 File "/home/travis/virtualenv/python3.5.6/lib/python3.5/shutil.py", line 418, in _rmtree_safe_fd
1252 _rmtree_safe_fd(dirfd, fullname, onerror)
1253 File "/home/travis/virtualenv/python3.5.6/lib/python3.5/shutil.py", line 438, in _rmtree_safe_fd
1254 onerror(os.unlink, fullname, sys.exc_info())
1255 File "/home/travis/virtualenv/python3.5.6/lib/python3.5/shutil.py", line 436, in _rmtree_safe_fd
1256 os.unlink(name, dir_fd=topfd)
1257OSError: [Errno 16] Device or resource busy: '.nfs00000000002b0b180000020a'

@mih mih mentioned this pull request Feb 21, 2020
2 tasks
@yarikoptic
Copy link
Member

yarikoptic commented Feb 21, 2020

Which is expected since you merged #4157 which introduced those failures and wasn't resolved.

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

mih commented Feb 21, 2020

Which is expected since you merged #4157 which introduced those failures and wasn't resolved.

No, those were present before and in other PRs.

@yarikoptic
Copy link
Member

yarikoptic commented Feb 21, 2020

Which is expected since you merged #4157 which introduced those failures and wasn't resolved.

No, those were present before and in other PRs.

hm, may be there was more from others... but FWIW nfs travis run and windows datalad.support.tests.test_annexrepo.test_find_batch_equivalence failures came up there first AFAIK

@mih
Copy link
Member Author

mih commented Feb 21, 2020

As noted above the NFS failure is #3929 in disguise AFAICS it -- which I reported in Dec 2019. It fails to remove a file that was never part of the report. #3929 has additional info.

@mih
Copy link
Member Author

mih commented Feb 25, 2020

More permission issues of the kind already pointed out here: #2518 filed as #4201

Uncovered test repo status/setup issue reported as #4202

@mih
Copy link
Member Author

mih commented Feb 26, 2020

A bunch of issues were uncovered by this PR. As far as I can see none of them was actually introduced here. I have filed dedicated issues for each of them (see above). Given the number of files touched, I will proceed with smaller follow-up PRs looking at individual issues. The only affected systems are Windows short-path-reporting machines, of which we have none in real life. They should not hold up development.

@mih
Copy link
Member Author

mih commented Feb 26, 2020

I cannot figure out why the NFS travis run stalls. I cannot reproduce on a real NFS server mount. We don't even report which tests are attempted. I have to focus on implementing a functional publish replacement now. Feel free to close this PR.

@mih
Copy link
Member Author

mih commented Feb 26, 2020

Found one more unused/leftover import in datalad/distribution/publish.py

@mih
Copy link
Member Author

mih commented Mar 4, 2020

So the stalling test is datalad.support.tests.test_annexrepo.test_annex_ssh, which primarily (or only) tests annex sync operation with SSH remotes.

@mih mih force-pushed the playme branch 2 times, most recently from d28d159 to 60236f8 Compare Mar 5, 2020
mih added 4 commits Mar 6, 2020
Defer import of GitPython and instantiation to the few pieces in
DataLad's tests that need a GitPython repo instance to work.
Things like ok_clean_git() still use it.
mih added 5 commits Mar 6, 2020
Was replaced by assert_repo_status() a long time ago.

With this change the last use of GitPython is gone.

A shim for ok_clean_git() is kept that maps the most common use cases
onto assert_repo_status() -- to be nice to extension devs.
Prevents a stall of `annex sync`(?) in
datalad.support.tests.test_annexrepo.test_annex_ssh on Travis that does
not replicate locally.
@mih
Copy link
Member Author

mih commented Mar 6, 2020

Given that the condition of the windows tests is unbearable already, I will no longer hold this. Once travis is happy and appveyor still is, I will merge this.

@yarikoptic
Copy link
Member

yarikoptic commented Mar 6, 2020

@joeyh - is there a way to install some previous version of git-annex on windows? I see only https://downloads.kitenet.net/git-annex/windows/current/ and no previous versions.

If we could adjust our windows tests workflows to use some previously working ok version, we could at least re-establish a stable testing ground, and then address windows breakages in a dedicated PR.

@kyleam
Copy link
Collaborator

kyleam commented Mar 6, 2020

If we could adjust our windows tests workflows to use some previously working ok version, we could at least re-establish a stable testing ground, and then address windows breakages in a dedicated PR.

At least some of the latest Windows failures popped up without any git-annex version change (all are 8.20200226-g2d3ef2c07). A change in the Git for Windows version is the main difference I spotted, though I'm not convinced that it's the culprit.

@yarikoptic
Copy link
Member

yarikoptic commented Mar 6, 2020

oh right - Windows installation of git-annex is not bundling git (unlike in osx, or git-annex-standalone in neurodebian), heh heh

@mih
Copy link
Member Author

mih commented Mar 6, 2020

I will merge this now. Windows tests are a different matter, and we still haven't reached a point where causal attribution of behavior changes is really a thing.

@mih mih merged commit cdc7d04 into datalad:master Mar 6, 2020
12 of 17 checks passed
@mih mih deleted the playme branch Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-if-ok
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants