Repository: lhotse-speech/lhotse

Issue:
LilcomChunkyWriter replaces storage_path if it contains "."
Hi! I realize that when i use storage_path with . like "gigaspeech_overlap_0.3_TRAIN" it will end up becoming saving to the file "gigaspeech_overlap_0.lca" instead of "gigaspeech_overlap_0.3_TRAIN.lca" as expected. 
It seems to be due to this line:
https://github.com/lhotse-speech/lhotse/blob/afc8eff1d1eaeeae42e0a628e1981bb231527340/lhotse/features/io.py#L829
which replaces the words after "." with lca instead of appending. 
I saw quite a few other places using `.with_suffix` as well so will have the same issue as well.

Discussion:
chiiyeh: I am not too sure what sort of test is needed, is it those similar to the one in the main testing folder like this:
https://github.com/lhotse-speech/lhotse/blob/master/test/features/test_chunky_writer.py#L31-L35
Also for the code referred in this test, not sure if there is a bug, as the `writer` variable is used at the `get_reader` outside of the `with` that it is created.
pzelasko: A test similar to that one would do. The goal is to cover a scenario in which the test would have failed before the change, but passes after it. You might want to use either `tmpdir` pytest fixture or python's `NamedTemporaryFile` to create a tmp file that has a problematic name. 

> Also for the code referred in this test, not sure if there is a bug, as the writer variable is used at the get_reader outside of the with that it is created.

That test is correct, `writer` still exists after `with` (at which point `writer.__exit__()` is being called), and we're only fetching the `.name` property to know which type of reader to create.
chiiyeh: Thanks for the clarification. Have added in the test. Tested it on my side, should be working.
chiiyeh: Gosh.. so sorry.. had trouble using the isort on my vscode.. and missed one of the changes. 

Patch:
diff --git a/lhotse/features/io.py b/lhotse/features/io.py
index bfcf268a..4a7b812a 100644
--- a/lhotse/features/io.py
+++ b/lhotse/features/io.py
@@ -280,7 +280,10 @@ class LilcomFilesWriter(FeaturesWriter):
         # too many files in a single directory.
         subdir = self.storage_path_ / key[:3]
         subdir.mkdir(exist_ok=True)
-        output_features_path = (subdir / key).with_suffix(".llc")
+        p = subdir / key
+        output_features_path = p.with_suffix(
+            p.suffix + ".llc" if p.suffix != ".llc" else ".llc"
+        )
         serialized_feats = lilcom.compress(value, tick_power=self.tick_power)
         with open(output_features_path, "wb") as f:
             f.write(serialized_feats)
@@ -343,7 +346,10 @@ class NumpyFilesWriter(FeaturesWriter):
         # too many files in a single directory.
         subdir = self.storage_path_ / key[:3]
         subdir.mkdir(exist_ok=True)
-        output_features_path = (subdir / key).with_suffix(".npy")
+        p = subdir / key
+        output_features_path = p.with_suffix(
+            p.suffix + ".npy" if p.suffix != ".npy" else ".npy"
+        )
         np.save(output_features_path, value, allow_pickle=False)
         # Include sub-directory in the key, e.g. "abc/abcdef.npy"
         return "/".join(output_features_path.parts[-2:])
@@ -450,7 +456,10 @@ class NumpyHdf5Writer(FeaturesWriter):
         check_h5py_installed()
         import h5py
 
-        self.storage_path_ = Path(storage_path).with_suffix(".h5")
+        p = Path(storage_path)
+        self.storage_path_ = p.with_suffix(
+            p.suffix + ".h5" if p.suffix != ".h5" else ".h5"
+        )
         self.hdf = h5py.File(self.storage_path, mode=mode)
 
     @property
@@ -539,7 +548,10 @@ class LilcomHdf5Writer(FeaturesWriter):
         check_h5py_installed()
         import h5py
 
-        self.storage_path_ = Path(storage_path).with_suffix(".h5")
+        p = Path(storage_path)
+        self.storage_path_ = p.with_suffix(
+            p.suffix + ".h5" if p.suffix != ".h5" else ".h5"
+        )
         self.hdf = h5py.File(self.storage_path, mode=mode)
         self.tick_power = tick_power
 
@@ -664,7 +676,10 @@ class ChunkedLilcomHdf5Writer(FeaturesWriter):
         check_h5py_installed()
         import h5py
 
-        self.storage_path_ = Path(storage_path).with_suffix(".h5")
+        p = Path(storage_path)
+        self.storage_path_ = p.with_suffix(
+            p.suffix + ".h5" if p.suffix != ".h5" else ".h5"
+        )
         self.tick_power = tick_power
         self.chunk_size = chunk_size
         self.hdf = h5py.File(self.storage_path, mode=mode)
@@ -826,7 +841,10 @@ class LilcomChunkyWriter(FeaturesWriter):
         assert mode in ("wb", "ab")
 
         # ".lca" -> "lilcom chunky archive"
-        self.storage_path_ = Path(storage_path).with_suffix(".lca")
+        p = Path(storage_path)
+        self.storage_path_ = p.with_suffix(
+            p.suffix + ".lca" if p.suffix != ".lca" else ".lca"
+        )
         self.tick_power = tick_power
         self.file = open(self.storage_path, mode=mode)
         self.curr_offset = self.file.tell()


Test patch:
diff --git a/test/features/test_feature_writer.py b/test/features/test_feature_writer.py
new file mode 100644
index 00000000..83b07cb2
--- /dev/null
+++ b/test/features/test_feature_writer.py
@@ -0,0 +1,81 @@
+from pathlib import Path
+from tempfile import TemporaryDirectory
+
+import numpy as np
+import pytest
+
+from lhotse import (
+    ChunkedLilcomHdf5Writer,
+    LilcomChunkyWriter,
+    LilcomFilesWriter,
+    LilcomHdf5Writer,
+    NumpyFilesWriter,
+    NumpyHdf5Writer,
+)
+from lhotse.utils import is_module_available
+
+
+@pytest.mark.parametrize(
+    ["writer_type", "ext"],
+    [
+        (LilcomFilesWriter, ".llc"),
+        (NumpyFilesWriter, ".npy"),
+    ],
+)
+def test_writer_saved_file(writer_type, ext):
+    # Generate small random numbers that are nicely compressed with lilcom
+    arr = np.log(np.random.uniform(size=(11, 80)).astype(np.float32) / 100)
+
+    with TemporaryDirectory() as d, writer_type(d) as writer:
+        # testing that words after . is not replace
+        input_key = "random0.3_vad.alpha"
+        key = writer.write(input_key, arr)
+        assert key == f"ran/{input_key}{ext}"
+
+        # Testing when end with extension it is not added again
+        input_key = f"temp0.2.alpha{ext}"
+        key = writer.write(input_key, arr)
+        assert key == f"tem/{input_key}"
+
+
+@pytest.mark.parametrize(
+    ["writer_type", "ext"],
+    [
+        pytest.param(
+            NumpyHdf5Writer,
+            ".h5",
+            marks=pytest.mark.skipif(
+                not is_module_available("h5py"),
+                reason="Requires h5py to run HDF5 tests.",
+            ),
+        ),
+        pytest.param(
+            LilcomHdf5Writer,
+            ".h5",
+            marks=pytest.mark.skipif(
+                not is_module_available("h5py"),
+                reason="Requires h5py to run HDF5 tests.",
+            ),
+        ),
+        pytest.param(
+            ChunkedLilcomHdf5Writer,
+            ".h5",
+            marks=pytest.mark.skipif(
+                not is_module_available("h5py"),
+                reason="Requires h5py to run HDF5 tests.",
+            ),
+        ),
+        (LilcomChunkyWriter, ".lca"),
+    ],
+)
+def test_chunk_writer_saved_file(writer_type, ext):
+    with TemporaryDirectory() as d:
+        # testing that words after . is not replace
+        filename = "random0.3_vad.alpha"
+        with writer_type(f"{d}/{filename}") as writer:
+            assert writer.storage_path_ == Path(f"{d}/{filename}{ext}")
+
+        # Testing when end with extension it is not added again
+        filename = f"random0.3_vad.alpha{ext}"
+        with writer_type(f"{d}/{filename}") as writer:
+            assert writer.storage_path_ == Path(f"{d}/{filename}")