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: Don't crash on no reported percentage #2891

Merged
merged 3 commits into from Oct 3, 2018

Conversation

Projects
None yet
2 participants
@bpoldrack
Member

bpoldrack commented Oct 3, 2018

This pull request fixes a failure discovered in https://github.com/datalad/datalad-container. Test was failing in https://travis-ci.org/datalad/datalad-container/jobs/436153807, because annex-addurl didn't report progress in the JSON report. Therefore ProcessAnnexProgressIndicators set the reported percentage to default to an empty string, which was then casted to a float, yielding a ValueError.

Independently on why exactly addurl doesn't report it (seems to just take a while for the first report to contain a field "percent-progress"), WE are setting the default to an empty string and therefore have to be able to deal with that value in the function we pass it to.

This pull request proposes to just catch the ValueError and return None in AnnexRepo._get_size_from_perc_complete().

Changes

  • This change is complete

Please have a look @datalad/developers

@bpoldrack bpoldrack force-pushed the bpoldrack:bf-addurl-progress branch from fed5bd0 to 8a300d3 Oct 3, 2018

@bpoldrack bpoldrack requested a review from yarikoptic Oct 3, 2018

@codecov

This comment has been minimized.

codecov bot commented Oct 3, 2018

Codecov Report

Merging #2891 into master will decrease coverage by 4.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2891      +/-   ##
==========================================
- Coverage   90.28%   86.23%   -4.06%     
==========================================
  Files         246      246              
  Lines       31932    31932              
==========================================
- Hits        28831    27535    -1296     
- Misses       3101     4397    +1296
Impacted Files Coverage Δ
datalad/support/annexrepo.py 87.34% <100%> (-0.88%) ⬇️
datalad/support/tests/test_annexrepo.py 94.77% <100%> (-1.59%) ⬇️
datalad/tests/utils_testdatasets.py 11.76% <0%> (-88.24%) ⬇️
datalad/interface/tests/test_annotate_paths.py 24% <0%> (-76%) ⬇️
datalad/distribution/tests/test_get.py 60.72% <0%> (-39.28%) ⬇️
datalad/metadata/tests/test_base.py 59.7% <0%> (-38.81%) ⬇️
datalad/plugin/tests/test_addurls.py 61.27% <0%> (-38.73%) ⬇️
...atalad/interface/tests/test_add_archive_content.py 66.66% <0%> (-33.34%) ⬇️
datalad/distribution/tests/test_update.py 73.75% <0%> (-26.25%) ⬇️
datalad/plugin/addurls.py 73.79% <0%> (-25.91%) ⬇️
... and 53 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 698e7ff...6c49c45. Read the comment docs.

try:
portion = (float(perc) / 100.)
except ValueError:
return None

This comment has been minimized.

@yarikoptic

yarikoptic Oct 3, 2018

Member

ATM it should be ok to introduce None as possible output here (at the use point it is or-ed with 0 after all), but I guess it would have been safer to just return 0 (although None is more semantically correct).
I am just afraid that if above code is changed under assumption that this one returns an int, we could be in trouble without realizing it (since there is no dedicated "integration" test for this use case).
So - I am ok with it as is, but ideally - should we just return 0 here (or just portion = None and leave return to logic below)

This comment has been minimized.

@bpoldrack

bpoldrack Oct 3, 2018

Member

(although None is more semantically correct)

That's my point, yes. But I'm fine with returning zero as well, if you consider this appropriate. And I agree to do so via portion=None. Will do.

bpoldrack added some commits Oct 3, 2018

@bpoldrack bpoldrack merged commit b0cfdbb into datalad:master Oct 3, 2018

8 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
datalad-pr-dl-osx-64 DEV build done.
Details
datalad-pr-docker-dl-nd14_04 DEV build done.
Details
datalad-pr-docker-dl-nd16_04 DEV build done.
Details
datalad-pr-docker-dl-nd80 DEV build done.
Details
datalad-pr-docker-dl-nd90 DEV build done.
Details

@yarikoptic yarikoptic modified the milestone: Release 0.10.4 Oct 22, 2018

yarikoptic added a commit that referenced this pull request Oct 24, 2018

Merge tag '0.11.0' into debian
 Upgrade of [git-annex] to the most recent available to your release is
 advisable since a number of issues were resolved at that level.

 ### Major refactoring and deprecations

 - `datalad.consts.LOCAL_CENTRAL_PATH` constant was deprecated in favor
   of `datalad.locations.default-dataset` [configuration] variable
   ([#2835])

 ### Minor refactoring

 - `"notneeded"` messages are no longer reported by default results
   renderer
 - [run] no longer shows commit instructions upon command failure when
   `explicit` is true and no outputs are specified ([#2922])
 - `get_git_dir` moved into GitRepo ([#2886])
 - `_gitpy_custom_call` removed from GitRepo ([#2894])
 - `GitRepo.get_merge_base` argument is now called `commitishes` instead
   of `treeishes` ([#2903])

 ### Fixes

 - [update] should not leave the dataset in non-clean state ([#2858])
   and some other enhancements ([#2859])
 - Fixed chunking of the long command lines to account for decorators
   and other arguments ([#2864])
 - Progress bar should not crash the process on some missing progress
   information ([#2891])
 - Default value for `jobs` set to be `"auto"` (not `None`) to take
   advantage of possible parallel get if in `-g` mode ([#2861])
 - [wtf] must not crash if `git-annex` is not installed etc ([#2865]),
   ([#2865]), ([#2918]), ([#2917])
 - Fixed paths (with spaces etc) handling while reporting annex error
   output ([#2892]), ([#2893])
 - `__del__` should not access `.repo` but `._repo` to avoid attempts
   for reinstantiation etc ([#2901])
 - Fix up submodule `.git` right in `GitRepo.add_submodule` to avoid
   added submodules being non git-annex friendly ([#2909]), ([#2904])
 - [run-procedure] ([#2905])
   - now will provide dataset into the procedure if called within dataset
   - will not crash if procedure is an executable without `.py` or `.sh`
     suffixes
 - Use centralized `.gitattributes` handling while setting annex backend
   ([#2912])
 - `GlobbedPaths.expand(..., full=True)` incorrectly returned relative
    paths when called more than once ([#2921])

 ### Enhancements and new features

 - Report progress on [clone] when installing from "smart" git servers
   ([#2876])
 - Stale/unused `sth_like_file_has_content` was removed ([#2860])
 - Enhancements to [search] to operate on "improved" metadata layouts
   ([#2878])
 - Output of `git annex init` operation is now logged ([#2881])
 - New
   - `GitRepo.cherry_pick` ([#2900])
   - `GitRepo.format_commit` ([#2902])
 - [run-procedure] ([#2905])
   - procedures can now recursively be discovered in subdatasets as well.
     The uppermost has highest priority
   - Procedures in user and system locations now take precedence over
     those in datasets.

* tag '0.11.0':
  Make it a 0.11.0 release since there were some API RFings
  REL: slight tune up to Changelog following the advices
  DOC: v0.10.4: Mention change in procedure precedence (a0cbcba)
  DOC: v0.10.4: Fix description of db715b7
  DOC: v0.10.4: Improve description of 6f615a4
  DOC: v0.10.4: Remove duplicate word

yarikoptic added a commit that referenced this pull request Oct 24, 2018

Merge tag '0.11.0' into debian
 ## 0.11.0 (Oct 23, 2018) -- Soon-to-be-perfect

 [git-annex] 6.20180913 (or later) is now required - provides a number of
 fixes for v6 mode operations etc.

 ### Major refactoring and deprecations

 - `datalad.consts.LOCAL_CENTRAL_PATH` constant was deprecated in favor
   of `datalad.locations.default-dataset` [configuration] variable
   ([#2835])

 ### Minor refactoring

 - `"notneeded"` messages are no longer reported by default results
   renderer
 - [run] no longer shows commit instructions upon command failure when
   `explicit` is true and no outputs are specified ([#2922])
 - `get_git_dir` moved into GitRepo ([#2886])
 - `_gitpy_custom_call` removed from GitRepo ([#2894])
 - `GitRepo.get_merge_base` argument is now called `commitishes` instead
   of `treeishes` ([#2903])

 ### Fixes

 - [update] should not leave the dataset in non-clean state ([#2858])
   and some other enhancements ([#2859])
 - Fixed chunking of the long command lines to account for decorators
   and other arguments ([#2864])
 - Progress bar should not crash the process on some missing progress
   information ([#2891])
 - Default value for `jobs` set to be `"auto"` (not `None`) to take
   advantage of possible parallel get if in `-g` mode ([#2861])
 - [wtf] must not crash if `git-annex` is not installed etc ([#2865]),
   ([#2865]), ([#2918]), ([#2917])
 - Fixed paths (with spaces etc) handling while reporting annex error
   output ([#2892]), ([#2893])
 - `__del__` should not access `.repo` but `._repo` to avoid attempts
   for reinstantiation etc ([#2901])
 - Fix up submodule `.git` right in `GitRepo.add_submodule` to avoid
   added submodules being non git-annex friendly ([#2909]), ([#2904])
 - [run-procedure] ([#2905])
   - now will provide dataset into the procedure if called within dataset
   - will not crash if procedure is an executable without `.py` or `.sh`
     suffixes
 - Use centralized `.gitattributes` handling while setting annex backend
   ([#2912])
 - `GlobbedPaths.expand(..., full=True)` incorrectly returned relative
    paths when called more than once ([#2921])

 ### Enhancements and new features

 - Report progress on [clone] when installing from "smart" git servers
   ([#2876])
 - Stale/unused `sth_like_file_has_content` was removed ([#2860])
 - Enhancements to [search] to operate on "improved" metadata layouts
   ([#2878])
 - Output of `git annex init` operation is now logged ([#2881])
 - New
   - `GitRepo.cherry_pick` ([#2900])
   - `GitRepo.format_commit` ([#2902])
 - [run-procedure] ([#2905])
   - procedures can now recursively be discovered in subdatasets as well.
     The uppermost has highest priority
   - Procedures in user and system locations now take precedence over
     those in datasets.

* tag '0.11.0':
  CHANGELOG adjusted to reflect new minimal version of git-annex
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment