Add a Hdf mixin that takes care of type info + general boilerplate #416

pmrv · 2021-08-26T13:44:13Z

It came up a few times that DataContainer was subclassed because of the HDF functionality, but wasn't otherwise very suitable, e.g. here and here and generally we have objects that write themselves to HDF but are not based on a general data structure like DataContainer. I've had this sketch for a while now that should make it easier to implement this kind of classes.

As a demonstration I've derived DataContainer from this new mixin, but generally it should be easy to move old classes to this format (expect GenericParameters which handles HDF in a slightly different way).

Test + docs will follow after discussion whether we find this useful.

niklassiemer · 2021-08-26T15:27:57Z

I really like this idea a lot! It might make writing new self-hdf-able classes a lot easier! Maybe you should add the @classmethod from_hdf_args(cls, hdf) somehow, since this one allows to pass in needed __init__ variables upon to_object().

liamhuber

I love this. WithHDF took me a second read-through to understand, but I find the use of __enter__ to handle the group name absolutely wonderful.

I posted a couple of minor suggestions to consider.

liamhuber · 2021-08-30T11:31:14Z

pyiron_base/interfaces/has_hdf.py

@@ -0,0 +1,63 @@
+from abc import ABC, abstractmethod
+
+class WithHDF:


Suggested change

class WithHDF:

class _WithHDF:

Or?

liamhuber · 2021-08-30T11:32:29Z

pyiron_base/interfaces/has_hdf.py

+    @abstractmethod
+    def _get_hdf_group_name(self):


Suggested change

@abstractmethod

def _get_hdf_group_name(self):

@property

@abstractmethod

def _hdf_group_name(self):

At least I think that's the right order for the decorators.

Am I missing a side effect?

It should work, I didn't do it before because it's a private method anyway, but I can change it now.

pmrv · 2021-08-30T13:12:08Z

pyiron_base/interfaces/has_hdf.py

+        hdf["HDF_VERSION"] = self.__hdf_version__
+
+    def from_hdf(self, hdf, group_name=None):
+        group_name = group_name or self._get_hdf_group_name()


Explicit check for None.

pmrv · 2021-08-30T13:15:22Z

pyiron_base/interfaces/has_hdf.py

+            self._to_hdf(hdf)
+            self._type_to_hdf(hdf)
+
+    def rewrite_hdf(self, hdf, group_name=None):


Garbage collection for HDF?

stale · 2021-09-13T14:33:19Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

pmrv · 2021-09-15T08:12:34Z

Fixed the test, details in the commits. The problem was that previously DataContainer.from_hdf didn't call hdf.close() if it descended into a group. HasHDF does that now, but since DataContainer passes a reference to HDFStub, this in turn reached into the wrong location in the file (since where hdf points to changes on hdf.close).

Adds a method to allow sub classes to specify an HDF group name that they want to be written into. Will be used if to_hdf() is not given a group_name. Pass self in abstractmethods

It's unused in all our code, but removing should involve discussion

This is so the objects implementation cannot muck it up accidentally.

Some objects only have an HDF version unlike jobs that also have a code version.

Not copying caused an error in lazy DataContainers, because with the new HasHDF inheritance it would: 1. hdf.open(...) and pass that to DataContainer._from_hdf 2. this method would then pass references to the HDF object to the HDFStub 3. HasHDF.from_hdf then calls hdf.close(), this modifies the hdf object passed to DataContainer in place, changing where it points to in the file 4. At some point HDFStub.load() would be called, but since the place where hdf points to changed under its feet HDFStub.load() then reaches into the wrong location in the file

pmrv · 2021-09-15T08:14:31Z

Rebased to resolve merge conflict.

liamhuber · 2021-09-15T15:01:20Z

Beautiful, tests are passing so I'm happy! If you have no further concerns, please merge it and let's get this thing live :)

Since PyironObjects are the trifecta of python, database, and file storage, we should consider if we want some sort of class Storable(HasHDF, HasStorage) as an intermediate that never needs to be located in the database.

EDIT: spelling of trifecta

pmrv · 2021-09-16T11:18:34Z

Beautiful, tests are passing so I'm happy! If you have no further concerns, please merge it and let's get this thing live :)

Since PyironObjects are the trifecta of python, database, and file storage, we should consider if we want some sort of class Storable(HasHDF, HasStorage) as an intermediate that never needs to be located in the database.

EDIT: spelling of trifecta

Since HasStorage just has the input/output container, I'd actually think, that it should just derive from HasHDF.

liamhuber · 2021-09-16T11:28:15Z

Since HasStorage just has the input/output container, I'd actually think, that it should just derive from HasHDF.

Yes, this can work. We just need to think a little bit whether (1) HasStorage._to_hdf calls self._storage.to_hdf(), or if (2) HasStorage overrides to_hdf with a super call and the storage saving.

(1) Pro: the ABC is satisfied and people can directly make new classes without ever worrying about storage (as long as everything they want serialized gets slapped onto the storage attribute. Con: If someone does want to save extra stuff they need to remember to call super in _to_hdf.

(2) Pro: _to_hdf maintains its status as "that thing I should do my own work in". Con: anyone inheriting from HasStorage is still stuck implementing _to/_from_hdf every single time, even if they don't need it.

I lean towards (1). Thoughts?

pmrv · 2021-09-16T11:49:12Z

Yeah, (1) sounds cleaner. Overloading the saving stuff should only be necessary for fundamental object and jobs anyway.

pmrv added the enhancement New feature or request label Aug 26, 2021

liamhuber approved these changes Aug 30, 2021

View reviewed changes

pmrv commented Aug 30, 2021

View reviewed changes

stale bot added the stale label Sep 13, 2021

liamhuber removed the stale label Sep 13, 2021

pmrv force-pushed the hdf_mixin branch from 345bde3 to 07282d9 Compare September 15, 2021 08:13

pmrv added 20 commits September 15, 2021 10:13

First stab at HDF mixin

47d1a69

Turn WithHDF into a slots object

ecf6ff3

Add _type_to_hdf to interface

51bb050

Implement _type_to_hdf

1e31f57

Pass HDF version to _from_hdf

abfd073

Move HasHDF to pyiron_base.interfaces

a8b42a3

Adds a method to allow sub classes to specify an HDF group name that they want to be written into. Will be used if to_hdf() is not given a group_name. Pass self in abstractmethods

Rename get_group_name method

3aedebe

Check if HDF group is empty before writing

61cce4f

Use _get_hdf_group_name in to_hdf

14e1319

Add HasHDF to DataContainer

94bc4d5

Keep old OBJECT field in type information

52b5bbb

It's unused in all our code, but removing should involve discussion

Save __version__ only if defined

9a14359

Implement rewrite_hdf

0f7cc6a

Write type info after calling _to_hdf

0b8c9c4

This is so the objects implementation cannot muck it up accidentally.

Fix import

25757bc

Make WithHDF private

6e52c70

Add from_hdf_args to Mixin

74072d6

Add copyright blurb

0da4c88

Don't define version by default

b95fb39

Some objects only have an HDF version unlike jobs that also have a code version.

Explicitly check for None

0a88211

pmrv force-pushed the hdf_mixin branch from 07282d9 to af26ef9 Compare September 15, 2021 08:14

pmrv added 5 commits September 16, 2021 10:25

Add type annotations

82a18a0

Add docstring and usage example

7ed52a6

More docstrings

27ff660

Talk about version

bb3b746

Run doctests

66464e8

pmrv merged commit 761483f into master Sep 16, 2021

delete-merged-branch bot deleted the hdf_mixin branch September 16, 2021 11:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a Hdf mixin that takes care of type info + general boilerplate #416

Add a Hdf mixin that takes care of type info + general boilerplate #416

pmrv commented Aug 26, 2021

niklassiemer commented Aug 26, 2021

liamhuber left a comment

liamhuber Aug 30, 2021

liamhuber Aug 30, 2021

pmrv Aug 30, 2021

pmrv Aug 30, 2021

pmrv Aug 30, 2021

stale bot commented Sep 13, 2021

pmrv commented Sep 15, 2021

pmrv commented Sep 15, 2021

liamhuber commented Sep 15, 2021 •

edited

Loading

pmrv commented Sep 16, 2021

liamhuber commented Sep 16, 2021

pmrv commented Sep 16, 2021

		@@ -0,0 +1,63 @@
		from abc import ABC, abstractmethod

		class WithHDF:

Add a Hdf mixin that takes care of type info + general boilerplate #416

Add a Hdf mixin that takes care of type info + general boilerplate #416

Conversation

pmrv commented Aug 26, 2021

niklassiemer commented Aug 26, 2021

liamhuber left a comment

Choose a reason for hiding this comment

liamhuber Aug 30, 2021

Choose a reason for hiding this comment

liamhuber Aug 30, 2021

Choose a reason for hiding this comment

pmrv Aug 30, 2021

Choose a reason for hiding this comment

pmrv Aug 30, 2021

Choose a reason for hiding this comment

pmrv Aug 30, 2021

Choose a reason for hiding this comment

stale bot commented Sep 13, 2021

pmrv commented Sep 15, 2021

pmrv commented Sep 15, 2021

liamhuber commented Sep 15, 2021 • edited Loading

pmrv commented Sep 16, 2021

liamhuber commented Sep 16, 2021

pmrv commented Sep 16, 2021

liamhuber commented Sep 15, 2021 •

edited

Loading