Move written flag from builder to HDF5IO, add tests

[rly]

Motivation

Required for the export PR: #326.

Currently, Builder objects have a field written that is used only by HDF5IO to mark whether the Builder object has been written to disk so that it does not get written multiple times in case of links and references. Builder objects should contain minimal state information such as whether it has been written, and this should really be information stored in the IO object. Refactoring to make HDF5IO track whether a builder has been written by that object will allow us to use a different HDF5IO instance to write the same builder to a different file.

Note:

• HDF5IO.__set_written is private because no other class should be able to change whether a builder has been written by this IO object. HDF5IO.get_written is public because other backends and code may want to be able to know whether a builder has been written to disk, but this could be made private.
• I replaced the test test_double_cache_spec with a new set of tests for writing/appending to check that the written flag is used correctly.

Close #380.

How to test the behavior?

See tests.

Checklist

• Have you checked our Contributing document?
• Have you ensured the PR clearly describes the problem and the solution?
• Is your contribution compliant with our coding style? This can be checked running flake8 from the source directory.
• Have you checked to ensure that there aren't other open Pull Requests for the same change?
• Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

[rly]

all branches of the if statement below had builder.written = True so I moved it to before the if statement

[oruebel]

The written flag should be set after the if/elif statements since that is when the state is modified. E.g., if an error occurs in require_dataset then the builder is falsely recorded as written.

[rly]

Ah, that makes sense.

[codecov]

Codecov Report

Merging #381 into hdmf_2.0 will not change coverage.
The diff coverage is 72.00%.

@@            Coverage Diff            @@
##           hdmf_2.0     #381   +/-   ##
=========================================
  Coverage     74.55%   74.55%           
=========================================
  Files            33       33           
  Lines          6633     6633           
  Branches       1447     1446    -1     
=========================================
  Hits           4945     4945           
  Misses         1279     1279           
  Partials        409      409           

Impacted Files	Coverage Δ
src/hdmf/build/builders.py	86.30% <ø> (-0.39%)	⬇️
src/hdmf/backends/hdf5/h5tools.py	67.56% <72.00%> (+0.37%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3fa0587...b586b41. Read the comment docs.

[rly]

Also consider the use case where a user writes an in-memory container to a file, then adds to the in-memory container, then writes the updated in-memory container to the same file using the following code:

    def test_write_append_unwritten(self):
        """Test writing a container, adding to the in-memory container, then writing it again in append mode."""
        with HDF5IO(self.path, manager=self.manager, mode='w') as io:
            io.write(self.foofile)

        # append new container
        foo3 = Foo('foo3', [10, 20], "I am foo3", 2, 0.1)
        new_bucket1 = FooBucket('new_bucket1', [foo3])
        self.foofile.buckets.append(new_bucket1)
        new_bucket1.parent = self.foofile

        # write to same file with same manager in APPEND mode
        with HDF5IO(self.path, manager=self.manager, mode='a') as io:
            io.write(self.foofile)

Previously this would succeed because the builder for self.foofile was already marked as written. Now it will fail with the h5py error ValueError: Unable to create group (name already exists) because the second IO object does not know that a group for self.foofile already exists in the file.

Although possibly confusing, I think this is OK behavior. Users should NOT use the above code to write an in-memory container and then append to it. Instead, they should do one of the following:

1. write the in-memory container only once after everything has been added,
2. on the second write, open the file in 'w' mode to overwrite the existing contents, or
3. open the file in 'a' mode after the first write, append to the read container, then call io.write

[ajtritt]

The same function is implemented here:

hdmf/src/hdmf/build/manager.py

Lines 193 to 194 in 3fa0587

	def __bldrhash__(self, obj):
	return id(obj)

Not sure if it should be generalized, by just adding __hash__ to Builder. It seems simple enough that its not a big deal to leave as is, but thought I would mention it.

[rly]

Yeah, that's where I got it from. I figured there might have been a reason that it was on the BuildManager rather than on Builder in the first place, e.g., maybe builders should by default have a hash that is based on its contents, so I left it and re-implemented it here.

[rly]

Note that since this technically includes a breaking change, this PR is being merged to a branch for HDMF 2.0, not dev.