Repository: IDSIA/sacred
Pull request #806 (2021-02-19 14:39:48)

FileStorageObserver: additional flags for saving sources and copying resources
At the moment `FileStorageObserver` will copy files into `_resources` even if they're already present elsewhere in `basedir/`. My particular use case is when resuming model training, in which the model's weights are saved as an artifact of one run, and used as a resource in a subsequent run.

This isn't necessarily desirable, for example if the model files are large.

This PR adds a `no_duplicate` option to `FileStorageObserver` that elides the copy into `basedir/_resources` if the source file is already present somewhere in `basedir/`. (Defaulting to making the copy as is already done.)

Additionally, I've found that I'm not fussed about saving sources, so as a convenience I've added a `should_save_sources` option that omits the copy into `basedir/_sources` if enabled. (Defaulting to saving sources as is already done.)

As a final point, I've found that the observer crashes if you try to add a resource "in-place", i.e. already where Sacred is going to put it. (Still a useful thing to do so that Sacred knows to track the file.) I do this to avoid unnecessarily creating a file, having Sacred copying it, and then having to delete it again -- meaningful overhead if the file is large. There's now a try-except to catch the error and allow the observer to continue operating.

I've added tests and updated documentation to match. Is this a PR you'd be interested in accepting?

patrick-kidger: Thanks, I'm glad to hear there's interest in this.

> I like this functionality and I can understand that it sometimes doesn't make sense to copy the files. But I see one problem with it: Sacred doesn't have control over the artifact files anymore. If you delete an artifact file (e.g., a model file from a past epoch) it is still listed in the run info but the file does not exist. This could make it hard if you want to load the artifacts later. Do you have an idea how to solve this?

I think the solution is "don't delete files from previous runs", in exactly the same way that the current approach mandates "don't delete files from `basedir/_resources`".
thequilo: Looks good to me. The failing test cases are addressed in #799.

The only thing that I noticed is that the `FileStorageObserver` uses internally still `str` and `os.path` instead of `pathlib`, but that's another thing.

Patch:
diff --git a/docs/observers.rst b/docs/observers.rst
index 67c4536..40e26fe 100644
--- a/docs/observers.rst
+++ b/docs/observers.rst
@@ -288,6 +288,13 @@ The FileStorageObserver also stores a snapshot of the source-code in a separate
 Their filenames are stored in the ``run.json`` file such that the corresponding
 files can be easily linked to their respective run.
 
+Storing source-code in this way can be disabled by passing
+``copy_sources=False`` when creating the FileStorageObserver. Copying any
+:ref:`resources` that are already present in `my_runs/`, but not present in
+`my_runs/_resources/` (for example, a resource that is the output of another
+run), can be disabled my passing ``copy_artifacts=False`` when creating the
+FileStorageObserver.
+
 Template Rendering
 ------------------
 In addition to these basic files, the FileStorageObserver can also generate a
diff --git a/requirements.txt b/requirements.txt
index 6f518e6..bd53325 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1,8 +1,8 @@
 docopt>=0.3, <1.0
-jsonpickle>=1.2, <2.0
+jsonpickle>=1.2
 munch>=2.0.2, <3.0
 wrapt>=1.0, <2.0
 py-cpuinfo>=4.0
 colorama>=0.4
 packaging>=18.0
-GitPython
\ No newline at end of file
+GitPython
diff --git a/sacred/observers/file_storage.py b/sacred/observers/file_storage.py
index 3ab9a5e..a9eeed6 100644
--- a/sacred/observers/file_storage.py
+++ b/sacred/observers/file_storage.py
@@ -8,7 +8,7 @@ from pathlib import Path
 from typing import Optional
 import warnings
 
-from shutil import copyfile
+from shutil import copyfile, SameFileError
 
 from sacred.commandline_options import cli_option
 from sacred.dependencies import get_digest
@@ -40,6 +40,8 @@ class FileStorageObserver(RunObserver):
         source_dir: Optional[PathType] = None,
         template: Optional[PathType] = None,
         priority: int = DEFAULT_FILE_STORAGE_PRIORITY,
+        copy_artifacts: bool = True,
+        copy_sources: bool = True,
     ):
         basedir = Path(basedir)
         resource_dir = resource_dir or basedir / "_resources"
@@ -53,7 +55,15 @@ class FileStorageObserver(RunObserver):
             template = basedir / "template.html"
             if not template.exists():
                 template = None
-        self.initialize(basedir, resource_dir, source_dir, template, priority)
+        self.initialize(
+            basedir,
+            resource_dir,
+            source_dir,
+            template,
+            priority,
+            copy_artifacts,
+            copy_sources,
+        )
 
     def initialize(
         self,
@@ -62,12 +72,16 @@ class FileStorageObserver(RunObserver):
         source_dir,
         template,
         priority=DEFAULT_FILE_STORAGE_PRIORITY,
+        copy_artifacts=True,
+        copy_sources=True,
     ):
         self.basedir = str(basedir)
         self.resource_dir = resource_dir
         self.source_dir = source_dir
         self.template = template
         self.priority = priority
+        self.copy_artifacts = copy_artifacts
+        self.copy_sources = copy_sources
         self.dir = None
         self.run_entry = None
         self.config = None
@@ -134,18 +148,21 @@ class FileStorageObserver(RunObserver):
         self.save_json(self.run_entry, "run.json")
         self.save_json(self.config, "config.json")
 
-        for s, m in ex_info["sources"]:
-            self.save_file(s)
+        if self.copy_sources:
+            for s, _ in ex_info["sources"]:
+                self.save_file(s)
 
         return os.path.relpath(self.dir, self.basedir) if _id is None else _id
 
     def save_sources(self, ex_info):
         base_dir = ex_info["base_dir"]
         source_info = []
-        for s, m in ex_info["sources"]:
+        for s, _ in ex_info["sources"]:
             abspath = os.path.join(base_dir, s)
-            store_path, md5sum = self.find_or_save(abspath, self.source_dir)
-            # assert m == md5sum
+            if self.copy_sources:
+                store_path = self.find_or_save(abspath, self.source_dir)
+            else:
+                store_path = abspath
             relative_source = os.path.relpath(str(store_path), self.basedir)
             source_info.append([s, relative_source])
         return source_info
@@ -180,14 +197,23 @@ class FileStorageObserver(RunObserver):
         return os.path.relpath(self.dir, self.basedir) if _id is None else _id
 
     def find_or_save(self, filename, store_dir: Path):
-        os.makedirs(str(store_dir), exist_ok=True)
-        source_name, ext = os.path.splitext(os.path.basename(filename))
-        md5sum = get_digest(filename)
-        store_name = source_name + "_" + md5sum + ext
-        store_path = store_dir / store_name
-        if not store_path.exists():
-            copyfile(filename, str(store_path))
-        return store_path, md5sum
+        try:
+            Path(filename).resolve().relative_to(Path(self.basedir).resolve())
+            is_relative_to = True
+        except ValueError:
+            is_relative_to = False
+
+        if is_relative_to and not self.copy_artifacts:
+            return filename
+        else:
+            store_dir.mkdir(parents=True, exist_ok=True)
+            source_name, ext = os.path.splitext(os.path.basename(filename))
+            md5sum = get_digest(filename)
+            store_name = source_name + "_" + md5sum + ext
+            store_path = store_dir / store_name
+            if not store_path.exists():
+                copyfile(filename, str(store_path))
+            return store_path
 
     def save_json(self, obj, filename):
         with open(os.path.join(self.dir, filename), "w") as f:
@@ -205,7 +231,10 @@ class FileStorageObserver(RunObserver):
                 "FileStorageObserver. "
                 "The list of blacklisted files is: {}".format(blacklist)
             )
-        copyfile(filename, dest_file)
+        try:
+            copyfile(filename, dest_file)
+        except SameFileError:
+            pass
 
     def save_cout(self):
         with open(os.path.join(self.dir, "cout.txt"), "ab") as f:
@@ -260,7 +289,7 @@ class FileStorageObserver(RunObserver):
         self.render_template()
 
     def resource_event(self, filename):
-        store_path, md5sum = self.find_or_save(filename, self.resource_dir)
+        store_path = self.find_or_save(filename, self.resource_dir)
         self.run_entry["resources"].append([filename, str(store_path)])
         self.save_json(self.run_entry, "run.json")
 


Test patch:
diff --git a/tests/test_observers/test_file_storage_observer.py b/tests/test_observers/test_file_storage_observer.py
index 6f4de34..fb74c7b 100644
--- a/tests/test_observers/test_file_storage_observer.py
+++ b/tests/test_observers/test_file_storage_observer.py
@@ -413,3 +413,35 @@ def test_blacklist_paths(tmpdir, dir_obs, sample_run):
     other_file.touch()
     with pytest.raises(FileExistsError):
         obs.save_file(str(other_file), "cout.txt")
+
+
+def test_no_duplicate(tmpdir, sample_run):
+    obs = FileStorageObserver(tmpdir, copy_artifacts=False)
+    file = Path(str(tmpdir / "koko.txt"))
+    file.touch()
+    obs.started_event(**sample_run)
+    obs.resource_event(str(file))
+    assert not os.path.exists(tmpdir / "_resources")
+
+    # Test the test: that the resource would otherwise have been created.
+    obs = FileStorageObserver(tmpdir, copy_artifacts=True)
+    sample_run["_id"] = sample_run["_id"] + "_2"
+    obs.started_event(**sample_run)
+    obs.resource_event(str(file))
+    assert os.path.exists(tmpdir / "_resources")
+    assert any(x.startswith("koko") for x in os.listdir(tmpdir / "_resources"))
+
+
+def test_no_sources(tmpdir, tmpfile, sample_run):
+    obs = FileStorageObserver(tmpdir, copy_sources=False)
+    sample_run["ex_info"]["sources"] = [[tmpfile.name, tmpfile.md5sum]]
+    obs.started_event(**sample_run)
+    assert not os.path.exists(tmpdir / "_sources")
+
+    # Test the test: that the source would otherwise have been created.
+    obs = FileStorageObserver(tmpdir, copy_sources=True)
+    sample_run["_id"] = sample_run["_id"] + "_2"
+    obs.started_event(**sample_run)
+    name, _ = os.path.splitext(os.path.basename(tmpfile.name))
+    assert os.path.exists(tmpdir / "_sources")
+    assert any(x.startswith(name) for x in os.listdir(tmpdir / "_sources"))