update release info for version 0.10.1 #474

dimitri-yatsenko · 2018-07-23T15:48:49Z

Fix Throw an error when create_virtual_module receives a non-existent schema/database #469
Prepared release 0.10.1

coveralls · 2018-07-23T15:53:05Z

Coverage decreased (-1.1%) to 88.22% when pulling 64c9e94 on dimitri-yatsenko:master into 49a93a7 on datajoint:master.

…t-python

ixcat

looks good from last week -
Did a change req unti we confirm the connection default to utf8 was the desired change.

ixcat · 2018-07-24T18:55:36Z

CHANGELOG.md

+* Sped up queries (#458)
+* Bugfix in restriction of the form (A & B) * B (#463)
+* Improved error messages (#466)



Should probably mention #468 - also:

Did we want to default the connection encoding to utf8 or latin1?

Utf8 means latin1 installs could potentially require adding a new setting with the release
(not sure if this is 100% compatible or 99.9% compatible and what the .01% would be if not)

This is a good point - could you tell us a bit more on what negative consequence might arise if one uses utf8 connection on a latin1 install of MySQL?

Generally I think this is probably one of the safer mismatches, However, I'm not sure of the precise impacts and it would probably take some time to investigate.

From what I could tell casually, it shouldn't cause any issues for the 'ascii range' (e.g. first 128 characters), but could potentially result in issues in the higher range of latin1 (https://en.wikipedia.org/wiki/ISO/IEC_8859-1, see characters > 128 value.).

Best set of general answers I found were here:
https://stackoverflow.com/questions/7048745/what-is-the-difference-between-utf-8-and-iso-8859-1/39109074

Of note, it looks like UTF8 for the characters in 128-255 range of latin1 is treated as multibyte; That is, the same 'glyph' would be translated for storage as 2 byte in the utf-8 scheme, but as a single byte in the 8859-1 (latin1) scheme. Where this translation occurs when configuring mysql connection and how it would relate to what is stored in a table, I'd need to test. (the thinking behind my previous not sure if this is 100% compatible or 99.9% compatible and what the .01% would be if not statement.)

my thought in making the initial comment was to assume there to be at least some limited impact (based on casual research as outlined above this appears true), and then treat the discussion here more as one of 'how to manage the presumed impact'; of course we can do tests and be more definitive for the nominal case of utf8 vs latin1 for the ascii range, or perhaps a bit beyond; that said, I'm not confident given our current knowledge of these topics that such tests should be relied upon (aka some other corner case could be missed & give false confidence in the decision).

Conversely, if we treat anything non-ascii as 'previously unsupported', the change should be fine; that said, no need to force a configuration setting if it is not required. Another option would be to default to setting nothing unless a setting is specified.

ixcat · 2018-07-24T18:56:30Z

setup.py

 here = path.abspath(path.dirname(__file__))

-long_description = "An object-relational mapping and relational algebra to facilitate data definition and data manipulation in MySQL databases."
+long_description = "A relational data framework for scientific data pipelines with MySQL backend."


sounds good :)

eywalker

Let's discuss what's an appropriate default for the encoding and make a release. I don't think make_module_code should be part of this release.

eywalker · 2018-08-16T13:42:04Z

datajoint/schema.py

            FROM information_schema.tables WHERE table_schema='{db}'
            """.format(db=self.database)).fetchone()[0])

+    def make_module_code(self):


What's this for? Addition of an entirely new feature like this should not be done as part of a patch increment. I'd much rather you add this to the next release.

…-existing schemas

eywalker · 2018-08-18T07:07:43Z

datajoint/__init__.py

    :return: the python module
    """
    mod = ModuleType(modulename)
-    s = schema(dbname, mod.__dict__)


while at this, can we make a few changes?

rename create_tables to create_schema in the Schema class - the name create_tables actually doesn't make sense as this really controls whether the schema is going to be created in the database or not

get rid of the mod.__dict__ - that will make this schema instance behave the same way as any one defined like schema = dj.schema('schema_name').

create_tables is already widely used and renaming it would cause problems. Usually, the user means to prevent the creation of new tables when the database schema already exists. I think it makes senes to keep things as they are now.

I do not believe that omitting mod.__dict__ will do what you describe. We are creating a module here, not a schema.

I believe the current code is correct as is.

After an off-line discussion, we agreed that create_if_missing will replace create_tables but in a future release.

eywalker · 2018-08-24T17:19:25Z

datajoint/__init__.py

    mod = ModuleType(modulename)
-    s = schema(dbname, mod.__dict__)
-    s.spawn_missing_classes()
+    s = schema(dbname, create_tables=False)


Does it need to be create_tables=False? Why not allow someone to use this to define new tables with this schema object? I prefer that we just leave it like how a typical schema object from other module would look like, which is create_tables=True

Good point and yes, I agree. However, let's make it configurable by adding a create_if_missing argument to create_virtual_module. Let's do it with the next feature release when we rename create_tables throughout.

I think create_if_missing should default to False for in create_virtual_module to avoid excessive schema creation due to simple mispellings.

Ok I agree that making it configurable would be best. But for now, shall we just remove the create_tables? I want the default to be create_tables=True (that is in future, create_if_missing=True to be the default).

I'm now thinking that there should be separate flags for create_schema and create_tables. I can see your point of wanting to avoid creating a schema if it doesn't already exist when using create_virtual_module, but I would still want to use it to create additional tables in the schema. So I think default should be create_schema=False, create_tables=True for create_virtual_module

Then we should add create_schema to Schema.__init__ too, defaulting to True.

Then we are going to keep create_tables as is, right?

I am okay with adding this change to this PR since it does not affect existing functionality.

…create_schema kwarg.

eywalker

One minor question about defaults - but otherwise looks great.

eywalker · 2018-08-27T17:40:32Z

datajoint/__init__.py



-def create_virtual_module(modulename, dbname):
+def create_virtual_module(module_name, schema_name, create_schema=False, create_tables=False):


What would be the reason to have create_tables default to False? I totally agree with making create_schema=False default but don't see strong reason to prevent usage of schema in defining additional tables in the schema?

I don’t have an opinion either way. This use case is rare and I think should be discouraged in favor of the more common approach using the dj.schema. Users can explicitly set create_tables=True if they do intend to add new tables.

I guess the idea is to treat the dj.create_virtual_module to be by default a "read-only" schema module? I guess that could make sense.

Alright have a couple more minutes of thought I don't have a strong reason against this default "read-only" behavior for virtual modules, especially given that user does have an option to set create tables to true.

Let's merge then and make a release.

dimitri-yatsenko added 2 commits July 23, 2018 10:47

update release info for version 0.10.1

c1d3bba

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

bc38a43

dimitri-yatsenko added 2 commits July 23, 2018 15:37

minor comment

f85e915

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

ef503b2

…t-python

dimitri-yatsenko requested a review from eywalker July 24, 2018 14:21

ixcat self-requested a review July 24, 2018 18:53

ixcat suggested changes Jul 24, 2018

View reviewed changes

dimitri-yatsenko added 3 commits August 15, 2018 21:09

add schema._get_module_code

9351260

make_module_code() now works correctly for Part tables

9d35b66

PEP8

3e2b1ea

eywalker suggested changes Aug 16, 2018

View reviewed changes

dimitri-yatsenko added 4 commits August 16, 2018 11:29

fix datajoint#469 -- dj.create_virtual_module throws an error for non…

791b8a1

…-existing schemas

fix typo

8ca0c30

minor

845e42f

fix typo

d8e2eba

eywalker suggested changes Aug 18, 2018

View reviewed changes

dimitri-yatsenko added 2 commits August 23, 2018 12:56

remove default charset encoding spec to allow system spec

f3e40e1

add context argument to schema.spawn_missing_classes

1cd1558

eywalker suggested changes Aug 24, 2018

View reviewed changes

rename database to schema_name in schema constructor and add the …

64c9e94

…create_schema kwarg.

eywalker suggested changes Aug 27, 2018

View reviewed changes

eywalker approved these changes Aug 27, 2018

View reviewed changes

ixcat approved these changes Aug 28, 2018

View reviewed changes

ixcat merged commit b13ffda into datajoint:master Aug 28, 2018



		def create_virtual_module(modulename, dbname):
		def create_virtual_module(module_name, schema_name, create_schema=False, create_tables=False):

update release info for version 0.10.1 #474

update release info for version 0.10.1 #474

Uh oh!

Conversation

dimitri-yatsenko commented Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ixcat left a comment

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!

eywalker left a comment

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!

dimitri-yatsenko Aug 23, 2018 • 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!

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!

eywalker Aug 24, 2018 • 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!

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!

eywalker left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dimitri-yatsenko commented Jul 23, 2018 •

edited

Loading

coveralls commented Jul 23, 2018 •

edited

Loading

dimitri-yatsenko Aug 23, 2018 •

edited

Loading

eywalker Aug 24, 2018 •

edited

Loading