Validate Repo Integrity #173

rlizzo · 2019-12-04T18:11:28Z

Motivation and Context

Why is this change required? What problem does it solve?:

Validate the integrity of repo data and commit history.

Description

Describe your changes in detail:

crytographic hash of all data (read from disk) and commit refs
validate integrity of commits, data, branches, and historical provenance.
working in manual tests, not automated yet.

Types of changes

What types of changes does your code introduce? Put an x in all the boxes that apply:

Documentation update
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Is this PR ready for review, or a work in progress?

Ready for review
Work in progress

How Has This Been Tested?

Put an x in the boxes that apply:

Current tests cover modifications made
New tests have been added to the test suite
Modifications were made to existing tests to support these changes
Tests may be needed, but they are not included when the PR was proposed
I don't know. Help!

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have signed (or will sign when prompted) the tensorwork CLA.
I have added tests to cover my changes.
All new and existing tests passed.

codecov · 2019-12-04T18:25:15Z

Codecov Report

Merging #173 into master will increase coverage by 0.18%.
The diff coverage is 97.45%.

@@            Coverage Diff             @@
##           master     #173      +/-   ##
==========================================
+ Coverage   95.22%   95.39%   +0.18%     
==========================================
  Files          66       69       +3     
  Lines       11881    12268     +387     
  Branches     1011     1067      +56     
==========================================
+ Hits        11313    11703     +390     
+ Misses        371      365       -6     
- Partials      197      200       +3

Impacted Files	Coverage Δ
src/hangar/backends/selection.py	`90.48% <ø> (ø)`	⬆️
src/hangar/metadata.py	`95.28% <100%> (ø)`	⬆️
tests/test_repo_integrity_verification.py	`100% <100%> (ø)`
src/hangar/constants.py	`100% <100%> (ø)`	⬆️
src/hangar/arrayset.py	`94.97% <100%> (-0.13%)`	⬇️
src/hangar/backends/hdf5_00.py	`91.44% <100%> (-0.23%)`	⬇️
src/hangar/remotes.py	`89.38% <100%> (+0.16%)`	⬆️
src/hangar/backends/remote_50.py	`92.5% <100%> (-0.98%)`	⬇️
src/hangar/remote/content.py	`93.2% <100%> (+0.07%)`	⬆️
src/hangar/records/hashmachine.py	`100% <100%> (ø)`	⬆️
... and 33 more

rlizzo · 2019-12-05T11:17:36Z

@hhsecond can you take a look at this? The tests aren't complete, it needs documentation, and there are a few corner cases to take care of, but I'd like your opinion on if I'm missing anything significant.

My TODO list right now is:

Handle cases where key / values are slightly malformed. We need to catch exceptions thrown by any parsing methods encountered on the way to the integrity module (I'll highlight concerning regions below)
Acquire the writer lock before running. These methods don't actually write anything, but it wouldn't be fun to deal with changing world state while the checks run.

Also, i'm not sure if I want to be raising a RuntimeError at every point some issue is found? I like the current implementation because:

We don't do any unnecessary work. As soon as an issue is encountered the world comes crashing down and everything stops.
Deliminating issues in the code via a prominent raise RuntimeError(msg) statement is rather nice in terms of readability. There's no guessing what the issue encountered is, or where it was found since theres a detailed error message right there.

HOWEVER, raising errors in this fashion means that it's not really possible to generate a list of issues should there be multiple things wrong . In addition to stopping at the first error it encounters, this doesn't return a machine readable description of the issue. In its current form, there's not any reasonable path towards effective logging or automated attempts to recover data.

These seem like issues for the future, but I'm not sure if you have other thoughts?

src/hangar/diagnostics/integrity.py

rlizzo · 2019-12-05T20:25:24Z

TODOS complete and tests finished. When you have a chance, would love to hear your thoughts @hhsecond. No rush.

lgtm-com · 2019-12-07T03:11:40Z

This pull request introduces 4 alerts when merging 68fbe4b into d267c0a - view on LGTM.com

new alerts:

4 for Module-level cyclic import

hhsecond

I had run through the code and found super minor tweaks we could do. I'll try to finish it before EOD

src/hangar/dataloaders/tfloader.py

src/hangar/diagnostics/integrity.py

src/hangar/repository.py

src/hangar/records/hashs.py

hhsecond · 2019-12-09T07:33:22Z

src/hangar/records/hashs.py

-        recs = self._traverse_all_hash_records()
-        out = list(map(parsing.hash_data_raw_key_from_db_key, recs))
+        recs = self._traverse_all_hash_records(keys=True, values=False)
+        out = list(map(hash_data_raw_key_from_db_key, recs))


list comprehension? Here and few other places where we have map -> list conversion

list comprehension is ~10% slower than mapping when the function is predefined. I've changed it where appropriate, but won't be making the change universally.

I think
list(map(somefun, recs)) > [somefun(val) for val in recs] > map(somefun, recs)
I mean, it's probably negligible but just saying if you want to do a comparison

lgtm-com · 2019-12-10T07:40:20Z

This pull request fixes 3 alerts when merging e38844f into d267c0a - view on LGTM.com

fixed alerts:

3 for Unused local variable

src/hangar/records/hashs.py

…ification chain. Also including last funcs params so that we can identify which particular record is the issue

lgtm-com · 2019-12-10T09:40:24Z

This pull request fixes 3 alerts when merging 98123e9 into d267c0a - view on LGTM.com

fixed alerts:

3 for Unused local variable

rlizzo added enhancement New feature or request WIP Don't merge; Work in Progress labels Dec 4, 2019

rlizzo added this to In progress in Release 0.5 Plan via automation Dec 4, 2019

rlizzo commented Dec 5, 2019

View reviewed changes

src/hangar/diagnostics/integrity.py Show resolved Hide resolved

rlizzo commented Dec 5, 2019

View reviewed changes

src/hangar/diagnostics/integrity.py Outdated Show resolved Hide resolved

rlizzo added this to the v0.5.0 milestone Dec 5, 2019

rlizzo force-pushed the validate-integrity branch from 58dc2cc to 711899d Compare December 5, 2019 19:43

tensorwerk deleted a comment from lgtm-com bot Dec 5, 2019

rlizzo added Awaiting Review Author has determined PR changes area nearly complete and ready for formal review. and removed WIP Don't merge; Work in Progress labels Dec 5, 2019

rlizzo self-assigned this Dec 5, 2019

rlizzo force-pushed the validate-integrity branch from 711899d to 1a1a1fc Compare December 5, 2019 20:28

tensorwerk deleted a comment from lgtm-com bot Dec 5, 2019

hhsecond requested changes Dec 9, 2019

View reviewed changes

Release 0.5 Plan automation moved this from In progress to Review in progress Dec 9, 2019

hhsecond reviewed Dec 10, 2019

View reviewed changes

src/hangar/records/hashs.py Outdated Show resolved Hide resolved

rlizzo added 5 commits December 10, 2019 04:27

added methods needed during repo integrity verification

977d09c

reporting exceptions that occur along any point of the integirity ver…

ce29aa8

…ification chain. Also including last funcs params so that we can identify which particular record is the issue

using decorator to catch and format parsing exceptions

df9595e

filled out the rest of the test suite

5322627

import sorting and parsing optimization

98123e9

rlizzo force-pushed the validate-integrity branch from e38844f to 98123e9 Compare December 10, 2019 09:28

rlizzo merged commit 519d63b into tensorwerk:master Dec 10, 2019

Release 0.5 Plan automation moved this from Review in progress to Done Dec 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate Repo Integrity #173

Validate Repo Integrity #173

rlizzo commented Dec 4, 2019 •

edited

Loading

codecov bot commented Dec 4, 2019 •

edited

Loading

rlizzo commented Dec 5, 2019 •

edited

Loading

rlizzo commented Dec 5, 2019

lgtm-com bot commented Dec 7, 2019

hhsecond left a comment

hhsecond Dec 9, 2019

rlizzo Dec 10, 2019

hhsecond Dec 10, 2019 •

edited

Loading

lgtm-com bot commented Dec 10, 2019

lgtm-com bot commented Dec 10, 2019

Validate Repo Integrity #173

Validate Repo Integrity #173

Conversation

rlizzo commented Dec 4, 2019 • edited Loading

Motivation and Context

Why is this change required? What problem does it solve?:

Description

Describe your changes in detail:

Types of changes

How Has This Been Tested?

Checklist:

codecov bot commented Dec 4, 2019 • edited Loading

Codecov Report

rlizzo commented Dec 5, 2019 • edited Loading

rlizzo commented Dec 5, 2019

lgtm-com bot commented Dec 7, 2019

hhsecond left a comment

Choose a reason for hiding this comment

hhsecond Dec 9, 2019

Choose a reason for hiding this comment

rlizzo Dec 10, 2019

Choose a reason for hiding this comment

hhsecond Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

lgtm-com bot commented Dec 10, 2019

lgtm-com bot commented Dec 10, 2019

rlizzo commented Dec 4, 2019 •

edited

Loading

codecov bot commented Dec 4, 2019 •

edited

Loading

rlizzo commented Dec 5, 2019 •

edited

Loading

hhsecond Dec 10, 2019 •

edited

Loading