implement dj.Bucket class to handle S3 external storage operations. by ixcat · Pull Request #358 · datajoint/datajoint-python

ixcat · 2017-08-21T14:50:58Z

Per S3 external fire desired feature in #204

Currently not hooked into actual dj code; awaiting base external file logic 1st.

S3 functionality implemented via 'boto3' package; unit tests are currently
MOCKED using 'moto' S3 mock library due to difficulties w/r/t credential mgmt.

Currently not hooked into actual dj code; awaiting base external file logic 1st. S3 functionality implemented via 'boto3' package; unit tests are currently MOCKED using 'moto' S3 mock library due to difficulties w/r/t credential mgmt.

ixcat · 2017-08-21T15:43:40Z

Apparently dependencies are not properly defined for the boto/moto libraries for the unit tests to work in the travis envirionment - investigating.

ixcat · 2017-08-21T16:06:27Z

Noticed I did not incorporate API as defined in #204 here. Will fix to match after I determine the issue with travis.

Current environment mis-pulls in a /usr/share/google.* copy of a file, which is not python 3x compatible, and so builds fail. See also: GoogleCloudPlatform/compute-image-packages#213 which outlines ubuntu cloud images are not updated.

coveralls · 2017-08-21T17:19:58Z

Coverage decreased (-0.4%) to 92.175% when pulling b2cae86 on ixcat:master into fb34601 on datajoint:master.

coveralls · 2017-08-21T17:19:58Z

Coverage decreased (-0.4%) to 92.175% when pulling b2cae86 on ixcat:master into fb34601 on datajoint:master.

This reverts commit 126d9fc.

coveralls · 2017-08-21T17:24:40Z

Coverage decreased (-0.4%) to 92.175% when pulling 9acfbd6 on ixcat:master into fb34601 on datajoint:master.

ixcat · 2017-08-21T17:39:22Z

On further thinking - not yet going to implement API as outlined in #204 until feedback; the items there might have deviated w/r/t 'base' external file implementation which is essentially the 'reference' spec.. any adjustments can be handled during actual pre-merge process.

eywalker · 2017-10-02T20:21:07Z

I suggest that we close this PR for now and make a new one when it is actually ready for review and merge.

dimitri-yatsenko · 2017-10-05T18:34:04Z

Actually let's accept it. It does not hurt anything. Our next release 0.9 will need to handle S3 buckets.

dimitri-yatsenko · 2017-10-05T18:35:25Z

datajoint/external.py

+        self.connect()
+        r = self._s3.Object(self._bucket, rpath).delete()
+        try:
+            if r['ResponseMetadata']['HTTPStatusCode'] == 204:


return r['ResponseMetadata']['HTTPStatusCode'] == 204

included in new diff.

dimitri-yatsenko · 2017-10-05T18:43:58Z

datajoint/external.py

+            self.connect()
+            self._s3.Object(self._bucket, rpath).load()
+        except ClientError as e:
+            if e.response['Error']['Code'] == "404":


I think the following would be better

if e.response['Error']['Code'] != "404": raise DataJointError('error checking remote file') return False

Had some thoughts about this when implementing - to some extent it depends on

a) consistency with API in other extfile stuff
b) notion of default processing vs exceptions

this call is asking 'if a file exists' -
False indicates it does not; an exception indicates the program was not able to successfully check.

Essentially this logic flips the S3 API to more closely mimic something like stat(2) so that things like:

if not mybucket.stat('/remote/path'): mybucket.put('/local/path', '/remote/path')

work rather than needing to do:

try: mybucket.stat('/remote/path') except aws.NoS3FileErrorExceptionGizmo: mybucket.put('/local/path','/remote/path') finally: raise DataJointError('your s3 doesnt work')

misunderstanding corrected, as is the code :)

dimitri-yatsenko · 2017-10-05T18:45:04Z

datajoint/external.py

+        try:
+            self._s3.Object(self._bucket, rpath).upload_file(lpath)
+        except:
+            raise DataJointError('Error uploading file')


make error message more informative

new diff incorporates file paths and stringified exception to all error messages.

dimitri-yatsenko · 2017-10-05T18:45:19Z

datajoint/external.py

+        try:
+            self._s3.Object(self._bucket, rpath).download_file(lpath)
+        except Exception as e:
+            raise DataJointError('file download error')


make error message more informative

new diff incorporates file paths and stringified exception to all error messages.

coveralls · 2017-10-20T03:25:00Z

Coverage decreased (-0.4%) to 92.039% when pulling 0772a41 on ixcat:master into 40bb5a7 on datajoint:master.

… try blocks

coveralls · 2017-10-20T03:33:34Z

Coverage decreased (-0.4%) to 92.047% when pulling 80c3b26 on ixcat:master into 40bb5a7 on datajoint:master.

dimitri-yatsenko · 2017-10-20T03:46:13Z

datajoint/external.py

-            else:
-                raise DataJointError('error checking remote file')
+            if e.response['Error']['Code'] != "404":
+                raise DataJointError('Error checking remote file', str(rpath),


Throughout the module, I think it's better to use formatted strings than string sums. For example, this line should be

raise DataJointError('Error checking remote file {p} ({e})'.format(p=rpath, e=e))

It's more explicit and efficient.

switched.. and agreed (assuming you are correct) for the general case of 'how to manage strings in the codebase'.. that said if we are worried about string processing efficiency within exception constructors we have bigger problems on our hands :D

disagree this is more explicit however (even if more efficient processing wise).. but in any event, no biggie and will go with the flow

coveralls · 2017-10-20T04:18:08Z

Coverage decreased (-0.4%) to 92.047% when pulling 30ade48 on ixcat:master into 40bb5a7 on datajoint:master.

coveralls · 2017-10-20T20:31:34Z

Coverage decreased (-0.4%) to 92.047% when pulling c8a4763 on ixcat:master into 40bb5a7 on datajoint:master.

Chris Turner added 2 commits August 21, 2017 11:51

.travis.yml: boto setting copied from moto cfg

126d9fc

Hack BOTO_CONFIG to work in travis.

b2cae86

Current environment mis-pulls in a /usr/share/google.* copy of a file, which is not python 3x compatible, and so builds fail. See also: GoogleCloudPlatform/compute-image-packages#213 which outlines ubuntu cloud images are not updated.

Revert ".travis.yml: boto setting copied from moto cfg"

9acfbd6

This reverts commit 126d9fc.

dimitri-yatsenko added this to the Release 0.9 milestone Oct 2, 2017

dimitri-yatsenko requested changes Oct 5, 2017

View reviewed changes

Chris Turner added 2 commits October 19, 2017 22:00

Merge remote-tracking branch 'origin/master' into ixcat

ff4f5fd

external.py: misc updates based on review of pr datajoint#358.

0772a41

external.py: consistently call connect in methods & do operations w/i…

80c3b26

… try blocks

dimitri-yatsenko requested changes Oct 20, 2017

View reviewed changes

external.py: flip exception constructor strings to ''.format()

30ade48

external.py: rename existing external.py to s3.py

c8a4763

eywalker approved these changes Oct 20, 2017

View reviewed changes

dimitri-yatsenko approved these changes Oct 20, 2017

View reviewed changes

eywalker merged commit 54cd18f into datajoint:master Oct 20, 2017

Conversation

ixcat commented Aug 21, 2017

Uh oh!

ixcat commented Aug 21, 2017

Uh oh!

ixcat commented Aug 21, 2017

Uh oh!

coveralls commented Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Aug 21, 2017

Uh oh!

coveralls commented Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ixcat commented Aug 21, 2017

Uh oh!

eywalker commented Oct 2, 2017

Uh oh!

dimitri-yatsenko commented Oct 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ixcat Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ixcat Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Aug 21, 2017 •

edited

Loading

coveralls commented Aug 21, 2017 •

edited

Loading

coveralls commented Oct 20, 2017 •

edited

Loading

coveralls commented Oct 20, 2017 •

edited

Loading

ixcat Oct 20, 2017 •

edited

Loading

ixcat Oct 20, 2017 •

edited

Loading

coveralls commented Oct 20, 2017 •

edited

Loading

coveralls commented Oct 20, 2017 •

edited

Loading