-
Notifications
You must be signed in to change notification settings - Fork 11
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
Copy dandietag code from dandi-cli #58
Conversation
Codecov Report
@@ Coverage Diff @@
## master #58 +/- ##
==========================================
+ Coverage 95.91% 96.07% +0.15%
==========================================
Files 11 13 +2
Lines 1053 1274 +221
==========================================
+ Hits 1010 1224 +214
- Misses 43 50 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
||
part_size = mb(64) | ||
|
||
if file_size > tb(5): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just an fyi that on amazon this is terabytes not tebibytes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having said that, it's possible that amazon's info is not consistent across services. but they are generally clear about TB/TiB nomenclature.
def mb(bytes_size: int) -> int: | ||
return bytes_size * 2 ** 20 | ||
|
||
|
||
def gb(bytes_size: int) -> int: | ||
return bytes_size * 2 ** 30 | ||
|
||
|
||
def tb(bytes_size: int) -> int: | ||
return bytes_size * 2 ** 40 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are mebi, gibi, and tebi - bytes. perhaps its a good time to consolidate these options in relation to aws.
# 5MB is the minimum part size allowed by S3 | ||
MIN_PART_SIZE = mb(5) | ||
# 5GB is the maximum part size allowed by S3 | ||
MAX_PART_SIZE = gb(5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the one piece of info that gives me pause in terms of what amazon is accepting, we should check on our large files what the size of each part is. because 5 GiB > 5 GB, so if 5GB is the limit then i would assume 5 GiB would not be accepted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would indeed be nice to clear it up, but I would keep it as is for the purpose of this PR to provide as smooth (no side-effects) transition from dandi-cli to dandischema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll file an issue. this can be merged without these changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs a tiny bit of extra tests
raise ValueError("Partial update extended past end of file") | ||
|
||
|
||
class ETagHashlike: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this class (and used .partial_update
) seems to lack tests code coverage -- we never had it for this portion or it was triggered by dandi-cli other functionality somehow? should be easy to add a dedicated unittest to ensure a more complete test coverage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test for ETagHashlike
added.
ok, let's get this merged -- test coverage is improved although not 100% yet, and dandi-cli errors are due to newer metadata shema version which is not yet supported by dandi-api |
Closes #57.