Fix memory over-provisioning (#17)

* Fix memory over-provisioning

* Update tests/unittests/fixtures/test_run_component.py

.github/workflows/ci.yml

@@ -48,4 +48,4 @@ jobs:
         bin/viash -h
     - name: Viash ns test
       run: |
-        ./bin/viash ns test -s tests/integration/ -p docker
+        ./bin/viash ns test -s tests/integration/ -p docker --memory 1GB

CHANGELOG.rst

@@ -2,6 +2,18 @@
 Changelog
 *********
 
+0.5.0 (5/10/2023)
+================
+
+Breaking Changes
+----------------
+
+* Fixed an issue with `run_component` assigning too much memory when using `viash test` with `--memory`. 
+  Viash rounds the values for the units that are bigger than the unit specified with `--memory` up to the nearest integer. 
+  (i.e. with `--memory 500GB`, `memory_tb` becomes `1`). Because `run_component` took the value for the largest unit, 
+  memory was being over-provisioned. The breaking change involves `run_component` using the value from the
+  smallest (instead of largest) unit when the memory resources are specified with multiple units.
+
 0.4.1 (4/10/2023)
 =================
 

tests/integration/test_run_component/test_script.py

@@ -1,5 +1,13 @@
 import pytest
 import sys
+from io import StringIO
+from contextlib import redirect_stdout
+
+### VIASH START # noqa: E266
+meta = {
+    "memory_gb": 6,
+}
+### VIASH END # noqa: E266
 
 
 def test_run_component(run_component):
@@ -14,4 +22,17 @@ def test_run_component(run_component):
 
 
 if __name__ == "__main__":
-    sys.exit(pytest.main([__file__], plugins=["viashpy"]))
+    temp_stdout = StringIO()
+    print(f"meta: {meta}")
+    assert (
+        meta["memory_gb"] is not None
+    ), "This test should be executed with some --memory set."
+    with redirect_stdout(temp_stdout):
+        result = pytest.main([__file__], plugins=["viashpy"])
+    stdout_str = temp_stdout.getvalue()
+    print(stdout_str)
+    assert (
+        "Different values were defined in the 'meta' dictionairy that limit memory, choosing the one with the smallest unit"
+        not in stdout_str
+    )
+    sys.exit(result)

tests/unittests/fixtures/test_run_component.py

@@ -86,13 +86,14 @@ def test_loading_run_component(run_component):
             6144,
             6291456,
             None,
-            3221225472,
+            6442450944,
             True,
         ),  # Memory specified and different, pick the largest
         (None, None, 6, None, None, None, 6442450944, False),  # Only one specified
         (None, None, 6.5, None, None, None, 6979321856, False),
-        (None, None, 3.5, 6144, 6291456, None, 3758096384, True),
+        (None, None, 3.5, 6144, 6291456, None, 6442450944, True),
         (None, None, 6, 6144.5, 6291456, None, 6442450944, True),
+        (None, None, 6, 6144.5, 6291456.5, None, 6442451456, True),
     ],
 )
 def test_run_component_different_memory_specification_warnings(
@@ -156,7 +157,7 @@ def test_loading_run_component(mocker, run_component):
     if expected_warning:
         result.stdout.fnmatch_lines(
             [
-                "*Different values were defined in the 'meta' dictionairy that limit memory, choosing the one with the largest unit.*"
+                "*Different values were defined in the 'meta' dictionary that limit memory, choosing the one with the smallest unit.*"
             ]
         )
     assert result.ret == 0

viashpy/testing.py

@@ -45,6 +45,15 @@ def cpus(meta_attribute_getter):
 
 @pytest.fixture
 def memory_bytes(meta_attribute_getter):
+    """
+    Cover the different scenarios that can occur when memory requirements are set.
+
+    1. All memory fields set to 'None' (i.e. viash (ns) test without `--memory` or all fields manually set to None in test script): return None
+    2. All memory fields not defined (i.e. KeyError, only when not set in test script and not using viash (ns) test): return None
+    3. All memory fields set (either by running viash (ns) test or setting them manually): return the bytes
+    4. Multiple values set, but not all (manually in test script): return the value for the smallest unit, converted to bytes.
+    5. 1 value set: return the value (manually in test script), converted to bytes.
+    """
     all_memory_attributes = {}
     for suffix in tobytesconverter.AVAILABLE_UNITS():
         try:
@@ -63,17 +72,19 @@ def memory_bytes(meta_attribute_getter):
         for suffix, memory in all_memory_attributes.items()
     }
     if len(set(memory_bytes.values())) > 1:
-        warnings.warn(
-            "Different values were defined in the 'meta' dictionairy that "
-            "limit memory, choosing the one with the largest unit."
-        )
-        largest_unit_value = None
+        if len(tobytesconverter.AVAILABLE_UNITS()) != len(memory_bytes):
+            # if not all units are set, the user probably specified the memory themselves multiple times...
+            warnings.warn(
+                "Different values were defined in the 'meta' dictionairy that "
+                f"limit memory, choosing the one with the smallest unit. Found: {memory_bytes}, "
+                f"available units: {tobytesconverter.AVAILABLE_UNITS()}."
+            )
         for unit in tobytesconverter.AVAILABLE_UNITS():
             try:
-                largest_unit_value = memory_bytes[unit]
+                return f"{int(memory_bytes[unit])}B"
             except KeyError:
                 pass
-        return f"{int(largest_unit_value)}B"
+        raise RuntimeError("At least one available unit should have been found.")
     (_, unit_value), *_ = memory_bytes.items()
     return f"{int(unit_value)}B"