Repository: BAMWelDX/weldx

Issue:
`WeldxFile` does not delete removed block data on update
Consider the following script where we update a file mostly by removing stuff:

~~~ python
import asdf
import numpy as np

from weldx import WeldxFile

d1 = np.ones((10, 3)) * 2
d2 = np.ones((3)) * 3
d3 = np.ones((17)) * 4
d4 = np.ones((10, 4)) * 5
d5 = np.ones(14)

trees = [
    {"d1": d1, "d2": d2, "d3": d3, "d4": d4},
    {"d1": d1, "d3": d3},
    {"d1": d1},
    {"d1": d1, "d5": d5},
    {"d1": d1, "d2": d2, "d5": d5},
    {"d3": d3},
]

# WeldxFile version
for tree in trees:
    WeldxFile("test.wx", mode="rw", tree=tree)

# AsdfFile version
asdf.AsdfFile(trees[0]).write_to("test.asdf")

for tree in trees[1:]:
    f = asdf.open("test.asdf", mode="rw")
    f.tree = tree
    f.update()
    f.close()

~~~

The resulting output of the `WeldxFile` version is:

~~~ yaml
#ASDF 1.0.0
#ASDF_STANDARD 1.5.0
%YAML 1.1
%TAG ! tag:stsci.edu:asdf/
--- !core/asdf-1.1.0
asdf_library: !core/software-1.0.0 {author: The ASDF Developers, homepage: 'http://github.com/asdf-format/asdf',
  name: asdf, version: 2.8.1}
history:
  extensions:
  - !core/extension_metadata-1.0.0
    extension_class: asdf.extension.BuiltinExtension
    software: !core/software-1.0.0 {name: asdf, version: 2.8.1}
d3: !core/ndarray-1.0.0
  source: 0
  datatype: float64
  byteorder: little
  shape: [17]
...
�BLK 0               �       �       �9���;�VP��AM&      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @#ASDF BLOCK INDEX
%YAML 1.1
---
- 503
...
O��&       @       @       @       @       @       @       @       @       @       @       @       @       @       @       @       @       @       @       @       @       @       @       @       @       @       @       @       @       @       @�BLK 0                             FQ!��U�RS��h�      @      @      @�BLK 0               p       p       p��7��4��:}|?A$L      �?      �?      �?      �?      �?      �?      �?      �?      �?      �?      �?      �?      �?      �?#ASDF BLOCK INDEX
%YAML 1.1
---
- 685
- 979
- 1057
...
    @      @      @      @      @      @      @      @�BLK 0              @      @      @L�o	$�L�f&">�
      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @#ASDF BLOCK INDEX
%YAML 1.1
---
- 778
- 1072
- 1150
- 1340
...
~~~

As you can see, the block data of previous writes is still kept in the file. After some testing, it seems that the full block is kept if data is removed. If data is added, it can overwrite bytes in the old blocks. For comparison, here is the `AsdfFile` version:

~~~ yaml
#ASDF 1.0.0
#ASDF_STANDARD 1.5.0
%YAML 1.1
%TAG ! tag:stsci.edu:asdf/
--- !core/asdf-1.1.0
asdf_library: !core/software-1.0.0 {author: The ASDF Developers, homepage: 'http://github.com/asdf-format/asdf',
  name: asdf, version: 2.8.1}
history:
  extensions:
  - !core/extension_metadata-1.0.0
    extension_class: asdf.extension.BuiltinExtension
    software: !core/software-1.0.0 {name: asdf, version: 2.8.1}
d3: !core/ndarray-1.0.0
  source: 0
  datatype: float64
  byteorder: little
  shape: [17]
...
ÓBLK 0               ˆ       ˆ       ˆ9öôÚ;ŸVP©ŒAM&      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @      @#ASDF BLOCK INDEX
%YAML 1.1
---
- 503
...
                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                  
~~~

It is not visible here, but the bytes of the last block are still in the file. They are just zeroized. In PyCharm you see a lot of "Zero" special characters, I don't know if this is intended or a bug that we might want to report upstream. Anyhow, so far I don't know why `WeldxFile` preserves the block data on deletions and if it is related to the `AsdfFile` behavior.

Discussion:
review-notebook-app[bot]: Check out this pull request on&nbsp; <a href="https://app.reviewnb.com/BAMWelDX/weldx/pull/539"><img align="absmiddle"  alt="ReviewNB" height="28" class="BotMessageButtonImage" src="https://raw.githubusercontent.com/ReviewNB/support/master/images/button_reviewnb.png"/></a> 

 See visual diffs & provide feedback on Jupyter Notebooks. 

---

 <i>Powered by <a href='https://www.reviewnb.com/?utm_source=gh'>ReviewNB</a></i>
pep8speaks: Hello @marscher! Thanks for updating this PR. 



* In the file [`weldx/asdf/util.py`](https://github.com/BAMWelDX/weldx/blob/8b6928b97b631f77a350c34ae199e415de4f1aa4/weldx/asdf/util.py):

> [Line 1:5](https://github.com/BAMWelDX/weldx/blob/8b6928b97b631f77a350c34ae199e415de4f1aa4/weldx/asdf/util.py#L1): [E999](https://duckduckgo.com/?q=pep8%20E999) SyntaxError: invalid syntax




marscher: > Hello @marscher! Thanks for updating this PR.
> 
>     * In the file [`weldx/asdf/util.py`](https://github.com/BAMWelDX/weldx/blob/8b6928b97b631f77a350c34ae199e415de4f1aa4/weldx/asdf/util.py):
> 
> 
> > [Line 1:5](https://github.com/BAMWelDX/weldx/blob/8b6928b97b631f77a350c34ae199e415de4f1aa4/weldx/asdf/util.py#L1): [E999](https://duckduckgo.com/?q=pep8%20E999) SyntaxError: invalid syntax

nope :)
codecov[bot]: # [Codecov](https://codecov.io/gh/BAMWelDX/weldx/pull/539?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX) Report
> Merging [#539](https://codecov.io/gh/BAMWelDX/weldx/pull/539?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX) (8b6928b) into [master](https://codecov.io/gh/BAMWelDX/weldx/commit/872a476bdaae8b97b1ad904302a6e8aed0116c88?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX) (872a476) will **increase** coverage by `0.03%`.
> The diff coverage is `95.23%`.

[![Impacted file tree graph](https://codecov.io/gh/BAMWelDX/weldx/pull/539/graphs/tree.svg?width=650&height=150&src=pr&token=wdof1qQTsn&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX)](https://codecov.io/gh/BAMWelDX/weldx/pull/539?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX)

```diff
@@            Coverage Diff             @@
##           master     #539      +/-   ##
==========================================
+ Coverage   95.12%   95.15%   +0.03%     
==========================================
  Files          93       93              
  Lines        5643     5660      +17     
==========================================
+ Hits         5368     5386      +18     
+ Misses        275      274       -1     
```


| [Impacted Files](https://codecov.io/gh/BAMWelDX/weldx/pull/539?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX) | Coverage Δ | |
|---|---|---|
| [weldx/asdf/util.py](https://codecov.io/gh/BAMWelDX/weldx/pull/539/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX#diff-d2VsZHgvYXNkZi91dGlsLnB5) | `86.45% <ø> (ø)` | |
| [weldx/asdf/file.py](https://codecov.io/gh/BAMWelDX/weldx/pull/539/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX#diff-d2VsZHgvYXNkZi9maWxlLnB5) | `98.29% <95.23%> (+0.59%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/BAMWelDX/weldx/pull/539?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/BAMWelDX/weldx/pull/539?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX). Last update [872a476...8b6928b](https://codecov.io/gh/BAMWelDX/weldx/pull/539?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX).


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index e3422f4..6d58b74 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -62,6 +62,8 @@
 - Merging and unmerging multiple `CoordinateSystemManager` instances now correctly preserves all attached data.
   [[#494]](https://github.com/BAMWelDX/weldx/pull/494).
 - `compare_nested` can compare sets [[#496]](https://github.com/BAMWelDX/weldx/pull/496)
+- `WeldxFile` cleans old memory blocks during file updates [[#539]](https://github.com/BAMWelDX/weldx/pull/539).
+- `WeldxFile` respects `mode` argument also for BytesIO and file handles [[#539]](https://github.com/BAMWelDX/weldx/pull/539).
 
 ### documentation
 
diff --git a/tutorials/GMAW_process.ipynb b/tutorials/GMAW_process.ipynb
index e816dc8..cd39342 100644
--- a/tutorials/GMAW_process.ipynb
+++ b/tutorials/GMAW_process.ipynb
@@ -137,7 +137,7 @@
    "metadata": {},
    "outputs": [],
    "source": [
-    "file = weldx.WeldxFile(tree=tree)\n",
+    "file = weldx.WeldxFile(tree=tree, mode=\"rw\")\n",
     "file.show_asdf_header()"
    ]
   },
diff --git a/tutorials/custom_metadata.ipynb b/tutorials/custom_metadata.ipynb
index 9ad6d65..f9fb709 100644
--- a/tutorials/custom_metadata.ipynb
+++ b/tutorials/custom_metadata.ipynb
@@ -68,7 +68,7 @@
    "outputs": [],
    "source": [
     "from weldx import WeldxFile\n",
-    "file = WeldxFile(tree={\"sensor\": HKS_sensor})\n",
+    "file = WeldxFile(tree={\"sensor\": HKS_sensor}, mode=\"rw\")\n",
     "file.show_asdf_header()"
    ]
   },
diff --git a/tutorials/groove_types_01.ipynb b/tutorials/groove_types_01.ipynb
index 7b0ead8..59e5c35 100644
--- a/tutorials/groove_types_01.ipynb
+++ b/tutorials/groove_types_01.ipynb
@@ -184,7 +184,7 @@
    "outputs": [],
    "source": [
     "tree = dict(test_v_groove=v_groove)\n",
-    "file = WeldxFile(tree=tree)"
+    "file = WeldxFile(tree=tree, mode=\"rw\")"
    ]
   },
   {
diff --git a/tutorials/measurement_chain.ipynb b/tutorials/measurement_chain.ipynb
index ccca358..b837658 100644
--- a/tutorials/measurement_chain.ipynb
+++ b/tutorials/measurement_chain.ipynb
@@ -285,7 +285,7 @@
    "cell_type": "markdown",
    "metadata": {},
    "source": [
-    "If we add an equipment to the `MeasurementChain`, it won't just only store the corresponding transformation but also remember the equipment that provides it. \n",
+    "If we add an equipment to the `MeasurementChain`, it won't just only store the corresponding transformation but also remember the equipment that provides it.\n",
     "We can get the linked equipment using `get_equipment`.\n",
     "Therefore we must provide the name of the transformation or source"
    ]
diff --git a/tutorials/measurement_example.ipynb b/tutorials/measurement_example.ipynb
index ff43782..5a5a28b 100644
--- a/tutorials/measurement_example.ipynb
+++ b/tutorials/measurement_example.ipynb
@@ -491,7 +491,7 @@
     "    \"data\": measurement_data,\n",
     "    \"measurements\": measurements,\n",
     "}\n",
-    "file = weldx.WeldxFile(tree=tree)"
+    "file = weldx.WeldxFile(tree=tree, mode=\"rw\")"
    ]
   },
   {
diff --git a/tutorials/welding_example_01_basics.ipynb b/tutorials/welding_example_01_basics.ipynb
index abcf1d6..e978260 100644
--- a/tutorials/welding_example_01_basics.ipynb
+++ b/tutorials/welding_example_01_basics.ipynb
@@ -552,7 +552,7 @@
    "metadata": {},
    "outputs": [],
    "source": [
-    "file = WeldxFile(tree=tree)"
+    "file = WeldxFile(tree=tree, mode=\"rw\")"
    ]
   },
   {
diff --git a/weldx/asdf/file.py b/weldx/asdf/file.py
index 5f386f5..45d2077 100644
--- a/weldx/asdf/file.py
+++ b/weldx/asdf/file.py
@@ -7,6 +7,7 @@ from contextlib import contextmanager
 from io import BytesIO, IOBase
 from typing import IO, Dict, List, Mapping, Optional, Union
 
+import asdf
 import numpy as np
 from asdf import AsdfFile, generic_io
 from asdf import open as open_asdf
@@ -114,7 +115,7 @@ class WeldxFile(UserDict):
     We define a simple data set and store it in a WeldxFile.
 
     >>> data = {"name": "CXCOMP", "value": 42}
-    >>> wx = WeldxFile(tree=data)
+    >>> wx = WeldxFile(tree=data, mode="rw")
 
     If we want to persist the WeldxFile to a file on hard drive we invoke:
 
@@ -199,6 +200,10 @@ class WeldxFile(UserDict):
             raise ValueError(
                 f'invalid mode "{mode}" given. Should be one of "r", "rw".'
             )
+        elif tree and mode != "rw":
+            raise RuntimeError(
+                "You cannot pass a tree (to be written) on a read-only file."
+            )
         self._mode = mode
         self.sync_upon_close = bool(sync) & (self.mode == "rw")
         self.software_history_entry = software_history_entry
@@ -220,6 +225,16 @@ class WeldxFile(UserDict):
                 self._in_memory = True
             else:
                 self._in_memory = False
+
+            # TODO: this could be inefficient for large files.
+            # For buffers is fast, but not for real files.
+            # real files should be probed over the filesystem!
+            if mode == "rw" and isinstance(
+                filename_or_file_like, SupportsFileReadWrite
+            ):
+                with reset_file_position(filename_or_file_like):
+                    new_file_created = len(filename_or_file_like.read()) == 0
+
             # the user passed a raw file handle, its their responsibility to close it.
             self._close = False
         else:
@@ -231,16 +246,21 @@ class WeldxFile(UserDict):
 
         # If we have data to write, we do it first, so a WeldxFile is always in sync.
         if tree or new_file_created:
-            asdf_file = AsdfFile(tree=tree, custom_schema=self.custom_schema)
-            asdf_file.write_to(filename_or_file_like, **write_kwargs)
+            asdf_file = self._write_tree(
+                filename_or_file_like,
+                tree,
+                asdffile_kwargs,
+                write_kwargs,
+                new_file_created,
+            )
             if isinstance(filename_or_file_like, SupportsFileReadWrite):
                 filename_or_file_like.seek(0)
-
-        asdf_file = open_asdf(
-            filename_or_file_like,
-            mode=self.mode,
-            **asdffile_kwargs,
-        )
+        else:
+            asdf_file = open_asdf(
+                filename_or_file_like,
+                mode=self.mode,
+                **asdffile_kwargs,
+            )
         self._asdf_handle: AsdfFile = asdf_file
 
         # UserDict interface: we want to store a reference to the tree, but the ctor
@@ -248,6 +268,27 @@ class WeldxFile(UserDict):
         super().__init__()
         self.data = self._asdf_handle.tree
 
+    def _write_tree(
+        self, filename_or_path_like, tree, asdffile_kwargs, write_kwargs, created
+    ) -> AsdfFile:
+        # cases:
+        # 1. file is empty (use write_to)
+        # 1.a empty buffer, iobase
+        # 1.b path pointing to empty (new file)
+        # 2. file exists, but should be updated with new tree
+        if created:
+            asdf_file = asdf.AsdfFile(tree=tree, **asdffile_kwargs)
+            asdf_file.write_to(filename_or_path_like, **write_kwargs)
+            generic_file = generic_io.get_file(filename_or_path_like, mode="rw")
+            asdf_file._fd = generic_file
+        else:
+            if self._mode != "rw":
+                raise RuntimeError("inconsistent mode, need to write data.")
+            asdf_file = open_asdf(filename_or_path_like, **asdffile_kwargs, mode="rw")
+            asdf_file.tree = tree
+            asdf_file.update(**write_kwargs)
+        return asdf_file
+
     @property
     def mode(self) -> str:
         """File operation mode: reading or reading/writing mode, one of "r" or "rw"."""
@@ -410,7 +451,7 @@ class WeldxFile(UserDict):
         (None, None, None)
         """
         tree = dict.fromkeys(iterable, default)
-        return WeldxFile(tree=tree)
+        return WeldxFile(tree=tree, mode="rw")
 
     def add_history_entry(self, change_desc: str, software: dict = None) -> None:
         """Add an history_entry to the file.
@@ -508,7 +549,7 @@ class WeldxFile(UserDict):
         Examples
         --------
         >>> tree = dict(wx_meta={"welder": "Nikolai Nikolajewitsch Benardos"})
-        >>> wf = WeldxFile(tree=tree)
+        >>> wf = WeldxFile(tree=tree, mode="rw")
         >>> wfa = wf.as_attr()
         >>> wfa.wx_meta.welder
         'Nikolai Nikolajewitsch Benardos'
diff --git a/weldx/asdf/util.py b/weldx/asdf/util.py
index 659cc36..4d0207d 100644
--- a/weldx/asdf/util.py
+++ b/weldx/asdf/util.py
@@ -118,7 +118,10 @@ def write_buffer(
         from weldx import WeldxFile
 
         with WeldxFile(
-            tree=tree, asdffile_kwargs=asdffile_kwargs, write_kwargs=write_kwargs
+            tree=tree,
+            asdffile_kwargs=asdffile_kwargs,
+            write_kwargs=write_kwargs,
+            mode="rw",
         ) as wx:
             wx.write_to()
             show(wx)


Test patch:
diff --git a/weldx/tests/asdf_tests/test_weldx_file.py b/weldx/tests/asdf_tests/test_weldx_file.py
index dfb31ae..53854cb 100644
--- a/weldx/tests/asdf_tests/test_weldx_file.py
+++ b/weldx/tests/asdf_tests/test_weldx_file.py
@@ -1,5 +1,6 @@
 """Tests for the WeldxFile class."""
 import itertools
+import os
 import pathlib
 import shutil
 import tempfile
@@ -136,7 +137,7 @@ class TestWeldXFile:
         """Test wrapper creation from a dictionary."""
         tree = dict(foo="bar")
         # creates a buffer
-        self.fh = WeldxFile(filename_or_file_like=None, tree=tree)
+        self.fh = WeldxFile(filename_or_file_like=None, tree=tree, mode="rw")
         new_file = self.make_copy(self.fh)
         assert WeldxFile(new_file)["foo"] == "bar"
 
@@ -172,11 +173,12 @@ class TestWeldXFile:
             assert fh2["foo"] == "bar"
             assert fh["another"] == "entry"
 
-    def test_create_writable_protocol(self):
+    @staticmethod
+    def test_create_writable_protocol():
         """Interface test for writable files."""
         f = WritableFile()
-        WeldxFile(f, tree=dict(test="yes"))  # this should write the tree to f.
-        new_file = self.make_copy(f.to_wrap)
+        WeldxFile(f, tree=dict(test="yes"), mode="rw")
+        new_file = TestWeldXFile.make_copy(f.to_wrap)
         assert WeldxFile(new_file)["test"] == "yes"
 
     @staticmethod
@@ -318,7 +320,7 @@ class TestWeldXFile:
         """Schema paths should be resolved internally."""
         schema = SINGLE_PASS_SCHEMA
         with pytest.raises(ValidationError) as e:
-            WeldxFile(custom_schema=schema)
+            WeldxFile(tree=dict(foo="bar"), custom_schema=schema, mode="rw")
         assert "required property" in e.value.message
 
     @staticmethod
@@ -466,3 +468,47 @@ class TestWeldXFile:
                 assert isinstance(e.value, FileExistsError)
         else:
             self.fh.copy(file, overwrite=overwrite)
+
+    @staticmethod
+    def test_update_existing_proper_update(tmpdir):
+        """Compare implementation of WeldxFile with asdf api.
+
+        WeldxFile should call update() to minimize memory usage."""
+        d1 = np.ones((10, 3)) * 2
+        d2 = np.ones(3) * 3
+        d3 = np.ones(17) * 4
+        d4 = np.ones((10, 4)) * 5
+        d5 = np.ones(14)
+        trees = [
+            {"d1": d1, "d2": d2, "d3": d3, "d4": d4},
+            {"d1": d1, "d3": d3},
+            {"d1": d1},
+            {"d1": d1, "d5": d5},
+            {"d1": d1, "d2": d2, "d5": d5},
+            {"d3": d3},
+        ]
+
+        os.chdir(tmpdir)
+        for tree in trees:
+            WeldxFile("test.wx", mode="rw", tree=tree)
+
+        # AsdfFile version
+        asdf.AsdfFile(trees[0]).write_to("test.asdf")
+
+        for tree in trees[1:]:
+            f = asdf.open("test.asdf", mode="rw")
+            f.tree = tree
+            f.update()
+            f.close()
+
+        # compare data
+        assert (
+            pathlib.Path("test.asdf").stat().st_size
+            == pathlib.Path("test.wx").stat().st_size
+        )
+
+        def _read(fn):
+            with open(fn, "br") as fh:
+                return fh.read()
+
+        assert _read("test.asdf") == _read("test.wx")