External storage, enhancements, bugfixes by dimitri-yatsenko · Pull Request #377 · datajoint/datajoint-python

dimitri-yatsenko · 2017-10-20T21:14:40Z

Here are the issues currently worked on. The issues that are fixed or in progress will be included in this pull request. The queued issues may be included.

Support AWS S3 storage of blobs #204 - fixed for filesystem, S3 still remains for a future pull request
Unions and Outer Joins #264 - fixed
Record the stack trace for errors in job table #328 - fixed
add optional progress report to populate #367 - fixed
Define limit in populate as number of keys to compute rathers than keys to check #375 - fixed
Deprecation warning for changed definition syntax. #381 - fixed
Should make replace _make_tuples? #387 - fixed
Improve skip_duplicates #388 - fixed
New union operator + #390 - fixed
Fetch with as_dict=True raises a confusing error when the attribute list is specified. #391 - fixed

…t-python

…in declarations

coveralls · 2017-11-05T09:57:16Z

Coverage decreased (-2.5%) to 89.414% when pulling 90c021e on dimitri-yatsenko:master into 8ea75a6 on datajoint:master.

coveralls · 2017-11-06T04:29:43Z

Coverage decreased (-2.4%) to 89.492% when pulling 7302571 on dimitri-yatsenko:master into 8ea75a6 on datajoint:master.

coveralls · 2017-11-06T04:33:00Z

Coverage decreased (-2.4%) to 89.492% when pulling 569881a on dimitri-yatsenko:master into 8ea75a6 on datajoint:master.

ixcat · 2017-11-06T22:39:51Z

Generally speaking, seems reasonable to me -
I'll be doing more direct testing as part of the S3 feature (#204)

thoughts:

If we are definitely leaving the external file cleanup as a separate action, it would be good to have a reference implementation of this, if it is even simple.
I think it would be a good idea to abstract the low-level external storage functions (currently get/put) into a separate class hierarchy that is called from the main ExternalTable operations. This will help to clearly encapsulate what methods are required to create ExternalTable 'backends', and also could allow for reuse among backends (e.g. S3 and any other other 'WAN' backends could concievably use operations defined by the plain-file backend to manage the local cache). I'd probably go with an abstract or plain-file class as base, and have a {'backendname' : BackendClass} dict statically configured as a class property of ExternalFile at least initially, but that's me.
Unless I am mistaken, the blob is currently being written outside of any serialization of operations on the specific external record file; this could conceivably create a race condition in some (rare) corner cases, though I am admittedly not yet 100% up to date on DJ's locking mechanisms and how they might relate to say, 2 processes writing the same external blob when using populate in parallel on different keys which generate the same externally-stored result in a related table.. For posix the data should be coherent (though possibly with incorrect timestamps if a slower writer finished after the fast writer overwrote it's data and committed the timestamp of the now-replaced-file to the db), but am also not sure if this is well defined on other systems or if their semantics would match. Quite possibly paranoia, but it might be a good idea to create a temporary '-lock' and check for the existence of this and either stall or generate an error if it exists, or create some sort of similar locking protocol to track this on the database side.. I think in previous discussions I had assumed external file transactions would be serialized; I'm now not sure this is happening and so am mentioning it here.
If the database insertion of the external file record into the database fails, we are left with a dangling blob. While this is a rare case and should be caught by the 'external cleanup' mechanism, it might be useful to handle cleanup here and then re-raise the error.

ixcat · 2017-11-07T15:13:22Z

datajoint/external.py

+
+        # serialize object
+        blob = pack(obj)
+        hash = long_hash(blob) + store[len('external-'):]


store[len('external-'):] seems incongruent w/r/t line 88: store = 'external' + ('-' if store else '') + store which could conceivably allow only external; that said using only 'external' as a specifier is already blocked in declare.py so I'm not sure how important this is.. perhaps line 88 should throw an error rather than allowing 'external' only?

The idea is to allow either external or external-somethin where somethin cannot exceed 8 characters. external by itself should not be blocked. If it is, it's my mistake and I will fix it and add a test for it. Many people will only have one external storage and they will just use external.

Why is store[len('external-'):] incongruent? You can do a[n:] where n exceeds the size of a no problem.

Ah ok - I think on first read I was taking this to mean there was an explicit expectation that the slice would return something to append to the hash, which was then "confirmed" when I saw that declare.py requires the external- convention (via the 'isidentifier' check on the secondary portion) and throws exceptions suggesting this is a requirement.

Not allowing the bare 'external' in some cases (here, declare.py) but allowing it in another (line 88) is why I proposed one was incongruent with respect to the other.

if 'external' alone is is allowed, it looks like the store_name parts from declare (line 191+) need to be altered -

>>> storestr='external' >>> storestr.split('-') ['external'] >>> store_name = storestr.split('-') >>> store_name[1:] [] >>> ''.join(store_name[1:]) '' >>> ''.join(store_name[1:]).isidentifier() False

I see. It should be isidentifier or empty.

ixcat · 2017-11-07T16:36:25Z

Concerning preparation for #204 -
In my s3 branch I've refactored the file handling operations into a separate class hierarchy which should help keep things more modular - the specific commit is:

ixcat@2729fd5

(other commits in that branch relate to other parts of S3 integration)

currently defined tests are passing locally.

If you agree with this approach, it might make sense to get that mechanism sorted out and merged in to your branch with this PR; alternately can tack it on afterwords. Either way good to get the discussion started.

eywalker · 2017-11-08T18:21:47Z

datajoint/relational_operand.py

                return arg, _negate
            elif isinstance(arg, AndList):
                return '(' + ' AND '.join([make_condition(element)[0] for element in arg]) + ')', _negate
+            elif arg is True or arg is False:


Much prefer isinstance(arg, bool)

eywalker · 2017-11-11T20:49:42Z

datajoint/declare.py

+            raise DataJointError('External attributes cannot be primary in:\n%s' % line)
+        store_name = match['type'].split('-')
+        if store_name[0] != 'external':
+            raise DataJointError('External store types must be in format external-<name>')


Should say in format external vs external-<name>

eywalker · 2017-11-11T20:55:30Z

datajoint/declare.py

+        if store_name[0] != 'external':
+            raise DataJointError('External store types must be in format external-<name>')
+        store_name = '-'.join(store_name[1:])
+        if not store_name.isidentifier():


This will reject external - needs to be updated to not (store_name == ' ' or storename.isidentifier())

eywalker · 2017-11-11T21:00:59Z

datajoint/relational_operand.py

-                "Cannot restrict in place a projection with renamed attributes."
-            if isinstance(restriction, AndList):
-                self.restrictions.extend(restriction)
+        :param as_or: if True than the applied restriction becomes or-listed with already existing conditions


This is documented but not present - either implement it or update the doc.

eywalker · 2017-11-11T21:01:59Z

datajoint/relational_operand.py

        return self & Not(restriction)

-    def restrict(self, restriction):
+    def restrict(self, arg):


I liked it better to call it restriction rather than arbitrary arg as the parameter name.

eywalker · 2017-11-11T21:05:48Z

tests/test_external.py

+from . schema_external import schema
+
+
+def test_external_put():


need to add test case for pure external in addition to external-xxx formatted configs

eywalker · 2017-11-11T21:06:26Z

tests/test_foreign_keys.py

@@ -1,5 +1,6 @@
 from nose.tools import assert_equal, assert_false, assert_true
 from datajoint.declare import declare
+from datajoint import AndList, OrList


why importing AndList and OrList here? Appears to be unused.

eywalker · 2017-11-11T21:31:18Z

datajoint/autopopulate.py

        logger.info('Found %d keys to populate' % len(keys))
-        for key in keys:
+
+        make = self._make_tuples if hasattr(self, '_make_tuples') else self.make


We should throw deprecation warning when we find _make_tuples

Concluded that we are holding off deprecation of _make_tuples

…ed PR commments.

coveralls · 2017-11-13T03:28:54Z

Coverage decreased (-1.8%) to 90.155% when pulling 86c2480 on dimitri-yatsenko:master into 8ea75a6 on datajoint:master.

… issue datajoint#391

coveralls · 2017-11-14T15:27:39Z

Coverage decreased (-1.8%) to 90.181% when pulling 6f7c6bd on dimitri-yatsenko:master into 8ea75a6 on datajoint:master.

coveralls · 2017-11-15T02:42:55Z

Coverage decreased (-1.8%) to 90.181% when pulling 8018a9d on dimitri-yatsenko:master into 8ea75a6 on datajoint:master.

… scaling problems

coveralls · 2017-11-15T12:20:38Z

Coverage decreased (-1.8%) to 90.181% when pulling 7627569 on dimitri-yatsenko:master into 8ea75a6 on datajoint:master.

dimitri-yatsenko added 29 commits October 12, 2017 13:36

Fixed datajoint#367

67e41ef

Merge branch 'master' of https://github.com/datajoint/datajoint-python

f89bf0e

fixed dependencies

159cb51

implemented the Union operator

bb88117

bugfix in populate

9703126

fixed a bug in server-side insert with missing attributes

4fa1abd

work on cascading delete

d6b9f2a

Merge branch 'master' of https://github.com/dimitri-yatsenko/datajoin…

c35ed7a

…t-python

fixed datajoint#375 -- added the max_calls argument to populate

7299058

simplified insert from select

3baf487

consolidating the use of hashes

4969c36

added module external for handling external storage

ce774be

minor fix in external, work in progress

ad650b0

minor

860d04b

changed how external storage is configured

2144b0f

minor bug

cfc1e21

minor formatting

0195cfc

removed the JobManager class -- it was never used

d821bcf

bugfix from previous commit

183b324

renamed the display_progress argument in populate()

6164eb5

Merge branch 'master' of https://github.com/dimitri-yatsenko/datajoin…

96f0d20

…t-python

rolled back unintended changes in delete

b37175b

fixed typo JobsTable -> JobTable

9688e5d

fixed bugs introduced in recent commits

bcc0048

Merge branch 'master' of https://github.com/datajoint/datajoint-python

28208c2

fixed test_requirements

04b1829

fixed indentation

1c8e071

minor

bb11c1b

minor

9fc36b4

dimitri-yatsenko changed the title ~~Fixed #264, #367, #375~~ [WIP] External storage, enhancements, bugfixes Oct 20, 2017

dimitri-yatsenko changed the title ~~[WIP] External storage, enhancements, bugfixes~~ External storage, enhancements, bugfixes Nov 5, 2017

dimitri-yatsenko added 3 commits November 5, 2017 03:26

followup to previous commit

383595d

made insert from query more consistent with insert from variables

794dc47

fixed issue datajoint#381 -- better error messages for syntax errors …

5ab3381

…in declarations

dimitri-yatsenko added 4 commits November 5, 2017 22:07

typo from previous commit

4b2671e

set the strict mode at connection time

9a0b902

set sql_mode in connection

7302571

updated the sql_mode

569881a

ixcat reviewed Nov 7, 2017

View reviewed changes

eywalker suggested changes Nov 11, 2017

View reviewed changes

added tests for union and for external storage. Other minor fixes bas…

86c2480

…ed PR commments.

improved documentation and error messages for fetch and fetch1. Fixed…

6f7c6bd

… issue datajoint#391

added tests for external storage

8018a9d

changed the shape of the computed nodes in the ERD to elipse to avoid…

7627569

… scaling problems

eywalker approved these changes Nov 15, 2017

View reviewed changes

eywalker merged commit 76e3d56 into datajoint:master Nov 15, 2017

This was referenced Nov 16, 2017

New union operator + #390

Closed

Should make replace _make_tuples? #387

Closed

Throw error for non-valid keyword arguments in fetch #343

Closed

Fetch with as_dict=True raises a confusing error when the attribute list is specified. #391

Closed

		from . schema_external import schema


		def test_external_put():

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

ixcat commented Nov 6, 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!

ixcat commented Nov 7, 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 Nov 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

coveralls commented Nov 15, 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

dimitri-yatsenko commented Oct 20, 2017 •

edited

Loading

coveralls commented Nov 5, 2017 •

edited

Loading

coveralls commented Nov 6, 2017 •

edited

Loading

coveralls commented Nov 6, 2017 •

edited

Loading

coveralls commented Nov 13, 2017 •

edited

Loading

coveralls commented Nov 14, 2017 •

edited

Loading

coveralls commented Nov 15, 2017 •

edited

Loading

coveralls commented Nov 15, 2017 •

edited

Loading