From 223bd5137cf3eb49506732e011a030dbc9845bdb Mon Sep 17 00:00:00 2001 From: Ryan Ly Date: Wed, 3 Aug 2022 14:43:40 -0700 Subject: [PATCH 1/9] Add _in_construct_mode to differentiate container init vs construct --- src/hdmf/build/objectmapper.py | 4 +++- src/hdmf/container.py | 5 +++++ tests/unit/test_container.py | 41 ++++++++++++++++++++++++++++------ 3 files changed, 42 insertions(+), 8 deletions(-) diff --git a/src/hdmf/build/objectmapper.py b/src/hdmf/build/objectmapper.py index 115ff3070..f095c60c9 100644 --- a/src/hdmf/build/objectmapper.py +++ b/src/hdmf/build/objectmapper.py @@ -1254,8 +1254,10 @@ def construct(self, **kwargs): def __new_container__(self, cls, container_source, parent, object_id, **kwargs): """A wrapper function for ensuring a container gets everything set appropriately""" - obj = cls.__new__(cls, container_source=container_source, parent=parent, object_id=object_id) + obj = cls.__new__(cls, container_source=container_source, parent=parent, object_id=object_id, + in_construct_mode=True) obj.__init__(**kwargs) + obj._in_construct_mode = False # reset to False after object construction return obj @docval({'name': 'container', 'type': AbstractContainer, diff --git a/src/hdmf/container.py b/src/hdmf/container.py index 45eee64b8..f9dbed906 100644 --- a/src/hdmf/container.py +++ b/src/hdmf/container.py @@ -176,6 +176,8 @@ def __gather_fields(cls, name, bases, classdict): cls.__fieldsconf = tuple(all_fields_conf) def __new__(cls, *args, **kwargs): + # NOTE: this method is called directly from ObjectMapper.__new_container__ during the process of + # constructing the object from builders that are read from a file inst = super().__new__(cls) if cls._experimental: warn(_exp_warn_msg(cls)) @@ -184,6 +186,9 @@ def __new__(cls, *args, **kwargs): inst.__children = list() inst.__modified = True inst.__object_id = kwargs.pop('object_id', str(uuid4())) + # this variable is being passed in from ObjectMapper.__new_container__ and is reset to False in that method + # after the object is initialized + inst._in_construct_mode = kwargs.pop('in_construct_mode', False) inst.parent = kwargs.pop('parent', None) return inst diff --git a/tests/unit/test_container.py b/tests/unit/test_container.py index f32897e70..a6d452910 100644 --- a/tests/unit/test_container.py +++ b/tests/unit/test_container.py @@ -1,4 +1,6 @@ import numpy as np +from uuid import uuid4, UUID + from hdmf.container import AbstractContainer, Container, Data from hdmf.testing import TestCase from hdmf.utils import docval @@ -10,22 +12,47 @@ class Subcontainer(Container): class TestContainer(TestCase): - def test_constructor(self): - """Test that constructor properly sets parent and both child and parent have an object_id + def test_new(self): + """Test that __new__ properly sets parent and other fields. """ parent_obj = Container('obj1') - child_obj = Container.__new__(Container, parent=parent_obj) + child_object_id = str(uuid4()) + child_obj = Container.__new__(Container, parent=parent_obj, object_id=child_object_id, + container_source="test_source") self.assertIs(child_obj.parent, parent_obj) self.assertIs(parent_obj.children[0], child_obj) - self.assertIsNotNone(parent_obj.object_id) - self.assertIsNotNone(child_obj.object_id) + self.assertEqual(child_obj.object_id, child_object_id) + self.assertFalse(child_obj._in_construct_mode) + self.assertTrue(child_obj.modified) - def test_constructor_object_id_none(self): - """Test that setting object_id to None in __new__ is OK and the object ID is set on get + def test_new_object_id_none(self): + """Test that passing object_id=None to __new__ is OK and results in a non-None object ID being assigned. """ parent_obj = Container('obj1') child_obj = Container.__new__(Container, parent=parent_obj, object_id=None) self.assertIsNotNone(child_obj.object_id) + UUID(child_obj.object_id, version=4) # raises ValueError if invalid + + def test_new_construct_mode(self): + """Test that passing in_construct_mode to __new__ sets _in_construct_mode and _in_construct_mode can be reset. + """ + parent_obj = Container('obj1') + child_obj = Container.__new__(Container, parent=parent_obj, object_id=None, in_construct_mode=True) + self.assertTrue(child_obj._in_construct_mode) + child_obj._in_construct_mode = False + self.assertFalse(child_obj._in_construct_mode) + + def test_init(self): + """Test that __init__ properly sets object ID and other fields. + """ + obj = Container('obj1') + self.assertIsNotNone(obj.object_id) + UUID(obj.object_id, version=4) # raises ValueError if invalid + self.assertFalse(obj._in_construct_mode) + self.assertTrue(obj.modified) + self.assertEqual(obj.children, tuple()) + self.assertIsNone(obj.parent) + self.assertEqual(obj.name, 'obj1') def test_set_parent(self): """Test that parent setter properly sets parent From 4042230702178b12124c1e5d4ecc95f865a3159d Mon Sep 17 00:00:00 2001 From: Ryan Ly Date: Wed, 3 Aug 2022 14:51:44 -0700 Subject: [PATCH 2/9] Update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e15d5b78..daa3de2f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ - Allow manual triggering of some GitHub Actions. @rly ([#744](https://github.com/hdmf-dev/hdmf/pull/744)) - Relax input validation of `HDF5IO` to allow for s3fs support. Existing arguments of `HDF5IO` are modified as follows: i) `mode` was given a default value of "r", ii) `path` was given a default value of `None`, and iii) `file` can now accept an `S3File` type argument. @bendichter ([#746](https://github.com/hdmf-dev/hdmf/pull/746)) - Added ability to create and get back handle to empty HDF5 dataset. @ajtritt (#747) +- Added `AbstractContainer._in_construct_mode` that is set and modified only by the ObjectMapper when constructing an + object from a builder read from a file. Subclasses of `AbstractContainer` can check `_in_construct_mode` to + determine whether to raise a warning or error when encountering invalid data (we want to be able to read and + fix data that is invalid but not create new data that is invalid). @rly (#751) ### Bug fixes - Fixed PyNWB dev CI. @rly ([#749](https://github.com/hdmf-dev/hdmf/pull/749)) From e314a5fa5afa587db4241485749a3d057d70e985 Mon Sep 17 00:00:00 2001 From: Ryan Ly Date: Wed, 3 Aug 2022 15:10:59 -0700 Subject: [PATCH 3/9] Fix unrelated flake8 issues due to upgrade of flake8 --- docs/gallery/plot_external_resources.py | 6 +++--- setup.cfg | 3 ++- tests/unit/test_io_hdf5_h5tools.py | 12 ++++++------ 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/docs/gallery/plot_external_resources.py b/docs/gallery/plot_external_resources.py index b9ad73df6..55c694634 100644 --- a/docs/gallery/plot_external_resources.py +++ b/docs/gallery/plot_external_resources.py @@ -505,13 +505,13 @@ table = pd.read_sql_query("SELECT * from %s" % table_name, db) table.set_index('id', inplace=True) ref_table = getattr(er, table_name).to_dataframe() - assert(np.all(np.array(table.index) == np.array(ref_table.index) + 1)) + assert np.all(np.array(table.index) == np.array(ref_table.index) + 1) for c in table.columns: # NOTE: SQLite uses 1-based row-indices so we need adjust for that if np.issubdtype(table[c].dtype, np.integer): - assert(np.all(np.array(table[c]) == np.array(ref_table[c]) + 1)) + assert np.all(np.array(table[c]) == np.array(ref_table[c]) + 1) else: - assert(np.all(np.array(table[c]) == np.array(ref_table[c]))) + assert np.all(np.array(table[c]) == np.array(ref_table[c])) cursor.close() ############################################################################### diff --git a/setup.cfg b/setup.cfg index 75418aa41..253d4ffc0 100644 --- a/setup.cfg +++ b/setup.cfg @@ -19,9 +19,10 @@ exclude = versioneer.py src/hdmf/_version.py src/hdmf/_due.py + docs/source/tutorials/ + docs/_build/ per-file-ignores = docs/gallery/*:E402,T001 - docs/source/tutorials/*:E402,T001 src/hdmf/__init__.py:F401 src/hdmf/backends/__init__.py:F401 src/hdmf/backends/hdf5/__init__.py:F401 diff --git a/tests/unit/test_io_hdf5_h5tools.py b/tests/unit/test_io_hdf5_h5tools.py index 100d96902..74342ebbd 100644 --- a/tests/unit/test_io_hdf5_h5tools.py +++ b/tests/unit/test_io_hdf5_h5tools.py @@ -1053,7 +1053,7 @@ def tearDown(self): # Close all the files for i in self.io: i.close() - del(i) + del i self.io = None self.f = None # Make sure the files have been deleted @@ -1282,7 +1282,7 @@ def setUp(self): def tearDown(self): if self.io is not None: self.io.close() - del(self.io) + del self.io if os.path.exists(self.path): os.remove(self.path) @@ -1316,7 +1316,7 @@ def setUp(self): def tearDown(self): if self.io is not None: self.io.close() - del(self.io) + del self.io if os.path.exists(self.path): os.remove(self.path) @@ -1358,7 +1358,7 @@ def setUp(self): def tearDown(self): if self.io is not None: self.io.close() - del(self.io) + del self.io if os.path.exists(self.path): os.remove(self.path) @@ -1390,7 +1390,7 @@ def setUp(self): def tearDown(self): if self.io is not None: self.io.close() - del(self.io) + del self.io if os.path.exists(self.path): os.remove(self.path) @@ -1459,7 +1459,7 @@ def setUp(self): def tearDown(self): if self.io is not None: self.io.close() - del(self.io) + del self.io if os.path.exists(self.path): os.remove(self.path) From 861a0cde54a09799b4941e01f97a2b4bd9ee603b Mon Sep 17 00:00:00 2001 From: Ryan Ly Date: Thu, 4 Aug 2022 10:14:24 -0700 Subject: [PATCH 4/9] Update CHANGELOG.md Co-authored-by: Oliver Ruebel --- CHANGELOG.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index daa3de2f9..bed5c6300 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,9 +7,11 @@ - Relax input validation of `HDF5IO` to allow for s3fs support. Existing arguments of `HDF5IO` are modified as follows: i) `mode` was given a default value of "r", ii) `path` was given a default value of `None`, and iii) `file` can now accept an `S3File` type argument. @bendichter ([#746](https://github.com/hdmf-dev/hdmf/pull/746)) - Added ability to create and get back handle to empty HDF5 dataset. @ajtritt (#747) - Added `AbstractContainer._in_construct_mode` that is set and modified only by the ObjectMapper when constructing an - object from a builder read from a file. Subclasses of `AbstractContainer` can check `_in_construct_mode` to - determine whether to raise a warning or error when encountering invalid data (we want to be able to read and - fix data that is invalid but not create new data that is invalid). @rly (#751) + object from a builder read from a file. Subclasses of `AbstractContainer` can check `_in_construct_mode` + during the initialization phase as part of ``__init__`` to distinguish between actions during construction + (i.e., read from disk) vs. creation by the user, e.g., to determine whether to raise a warning or error when + encountering invalid data to support reading and correcting data that is invalid while preventing creation + of new data that is invalid. @rly (#751) ### Bug fixes - Fixed PyNWB dev CI. @rly ([#749](https://github.com/hdmf-dev/hdmf/pull/749)) From 597d1de4a2d795931e48b73dfcf5df2c8fe92328 Mon Sep 17 00:00:00 2001 From: Ryan Ly Date: Thu, 4 Aug 2022 10:20:46 -0700 Subject: [PATCH 5/9] Update src/hdmf/build/objectmapper.py Co-authored-by: Oliver Ruebel --- src/hdmf/build/objectmapper.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hdmf/build/objectmapper.py b/src/hdmf/build/objectmapper.py index f095c60c9..7533fcfa4 100644 --- a/src/hdmf/build/objectmapper.py +++ b/src/hdmf/build/objectmapper.py @@ -1257,7 +1257,7 @@ def __new_container__(self, cls, container_source, parent, object_id, **kwargs): obj = cls.__new__(cls, container_source=container_source, parent=parent, object_id=object_id, in_construct_mode=True) obj.__init__(**kwargs) - obj._in_construct_mode = False # reset to False after object construction + obj._in_construct_mode = False # reset to False to indicate that the construction of the object is complete return obj @docval({'name': 'container', 'type': AbstractContainer, From d09cef25ac8b1fa334de4327c5efafa3984d860b Mon Sep 17 00:00:00 2001 From: Ryan Ly Date: Thu, 4 Aug 2022 10:21:38 -0700 Subject: [PATCH 6/9] Update src/hdmf/build/objectmapper.py Co-authored-by: Oliver Ruebel --- src/hdmf/build/objectmapper.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/hdmf/build/objectmapper.py b/src/hdmf/build/objectmapper.py index 7533fcfa4..a9e3cf8e6 100644 --- a/src/hdmf/build/objectmapper.py +++ b/src/hdmf/build/objectmapper.py @@ -1256,6 +1256,8 @@ def __new_container__(self, cls, container_source, parent, object_id, **kwargs): """A wrapper function for ensuring a container gets everything set appropriately""" obj = cls.__new__(cls, container_source=container_source, parent=parent, object_id=object_id, in_construct_mode=True) + # obj has been created and is in construction mode, indicating that the object is being constructed by + # the automatic construct process during read, rather than by the user obj.__init__(**kwargs) obj._in_construct_mode = False # reset to False to indicate that the construction of the object is complete return obj From 892efde6e321d92ad037c282e13ec91086816642 Mon Sep 17 00:00:00 2001 From: Ryan Ly Date: Thu, 4 Aug 2022 10:22:17 -0700 Subject: [PATCH 7/9] Update src/hdmf/container.py Co-authored-by: Oliver Ruebel --- src/hdmf/container.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/hdmf/container.py b/src/hdmf/container.py index f9dbed906..72edee331 100644 --- a/src/hdmf/container.py +++ b/src/hdmf/container.py @@ -176,8 +176,13 @@ def __gather_fields(cls, name, bases, classdict): cls.__fieldsconf = tuple(all_fields_conf) def __new__(cls, *args, **kwargs): - # NOTE: this method is called directly from ObjectMapper.__new_container__ during the process of - # constructing the object from builders that are read from a file + """ + Static method of the object class called by Python to create the object first and then + __init__() is called to initialize the object's attributes. + + NOTE: this method is called directly from ObjectMapper.__new_container__ during the process of + constructing the object from builders that are read from a file. + """ inst = super().__new__(cls) if cls._experimental: warn(_exp_warn_msg(cls)) From 403cd5deda9a6e7ca36bc0d3011132bc786d6ae4 Mon Sep 17 00:00:00 2001 From: Ryan Ly Date: Thu, 4 Aug 2022 10:22:52 -0700 Subject: [PATCH 8/9] Update src/hdmf/container.py Co-authored-by: Oliver Ruebel --- src/hdmf/container.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/hdmf/container.py b/src/hdmf/container.py index 72edee331..d2feb8839 100644 --- a/src/hdmf/container.py +++ b/src/hdmf/container.py @@ -191,8 +191,8 @@ def __new__(cls, *args, **kwargs): inst.__children = list() inst.__modified = True inst.__object_id = kwargs.pop('object_id', str(uuid4())) - # this variable is being passed in from ObjectMapper.__new_container__ and is reset to False in that method - # after the object is initialized + # this variable is being passed in from ObjectMapper.__new_container__ and is + # reset to False in that method after the object has been initialized by __init__ inst._in_construct_mode = kwargs.pop('in_construct_mode', False) inst.parent = kwargs.pop('parent', None) return inst From 37505cd8214aace42ec5ec44403932b46966bd37 Mon Sep 17 00:00:00 2001 From: Ryan Ly Date: Thu, 4 Aug 2022 10:32:07 -0700 Subject: [PATCH 9/9] Fix flake8 --- src/hdmf/container.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/hdmf/container.py b/src/hdmf/container.py index d2feb8839..b00b91145 100644 --- a/src/hdmf/container.py +++ b/src/hdmf/container.py @@ -179,7 +179,7 @@ def __new__(cls, *args, **kwargs): """ Static method of the object class called by Python to create the object first and then __init__() is called to initialize the object's attributes. - + NOTE: this method is called directly from ObjectMapper.__new_container__ during the process of constructing the object from builders that are read from a file. """ @@ -191,7 +191,7 @@ def __new__(cls, *args, **kwargs): inst.__children = list() inst.__modified = True inst.__object_id = kwargs.pop('object_id', str(uuid4())) - # this variable is being passed in from ObjectMapper.__new_container__ and is + # this variable is being passed in from ObjectMapper.__new_container__ and is # reset to False in that method after the object has been initialized by __init__ inst._in_construct_mode = kwargs.pop('in_construct_mode', False) inst.parent = kwargs.pop('parent', None)