Repository: MatterMiners/tardis

Issue:
Slurm siteadapter fails with fractional GB of memory
@wiene and I found that trying to define drones with non-integer `Memory` in their `MachineMetaData` works fine with most siteadapters (e.g. HTCondor), but breaks with Slurm:
```
2021-07-11 03:32:12 [WARNING][cobald.runtime.tardis.adapters.sites.slurm]: Execute command failed: 
(message=Run command sbatch -p medium -N 1 -n 1 -t 2880 --gres=scratch:4G --qos=low --mem=2.5gb --export=SLURM_Walltime=2880,TardisDroneCores=1,TardisDroneMemory=2560.0,TardisDroneDisk=4194304,TardisDroneUuid=uni-bonn.bonna-b76b556e52 /home/physik-services/drones/launch-atlas-drone via SSHExecutor
failed, exit_code=255, stdout=, stderr=sbatch: error: Invalid --mem specification
, stdin=None)
```
=> It seems `sbatch` (and `srun`) do not accept floating point values :cry: . 

One solution would be to change:
https://github.com/MatterMiners/tardis/blob/ae5320255759c4ff626175985ee2d21f8f3729c9/tardis/adapters/sites/slurm.py#L193
to use `mb` as unit, and multiply by `1024`, and explicitly convert to integer to prevent submit failure. 

Discussion:
codecov-commenter: # [Codecov](https://codecov.io/gh/MatterMiners/tardis/pull/197?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MatterMiners) Report
> Merging [#197](https://codecov.io/gh/MatterMiners/tardis/pull/197?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MatterMiners) (3fa8c87) into [master](https://codecov.io/gh/MatterMiners/tardis/commit/5e4a9356d61b94e0285b50ae985a0653d4047572?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MatterMiners) (5e4a935) will **not change** coverage.
> The diff coverage is `n/a`.

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

```diff
@@           Coverage Diff           @@
##           master     #197   +/-   ##
=======================================
  Coverage   99.54%   99.54%           
=======================================
  Files          42       42           
  Lines        1765     1765           
=======================================
  Hits         1757     1757           
  Misses          8        8           
```


| [Impacted Files](https://codecov.io/gh/MatterMiners/tardis/pull/197?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MatterMiners) | Coverage Δ | |
|---|---|---|
| [tardis/adapters/sites/slurm.py](https://codecov.io/gh/MatterMiners/tardis/pull/197/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MatterMiners#diff-dGFyZGlzL2FkYXB0ZXJzL3NpdGVzL3NsdXJtLnB5) | `100.00% <ø> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/MatterMiners/tardis/pull/197?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MatterMiners).
> **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=MatterMiners)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/MatterMiners/tardis/pull/197?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MatterMiners). Last update [5e4a935...3fa8c87](https://codecov.io/gh/MatterMiners/tardis/pull/197?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MatterMiners). 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=MatterMiners).

olifre: Thanks, that looks great!
I took the time to check the Slurm sources, and indeed:
https://github.com/SchedMD/slurm/blob/3e7b49a16cc7403245a2d8d7bd1f2b0df3228128/src/common/slurm_protocol_defs.c#L6022
* `gb` uses 1000 multipliers
* `g` and `gib` use `1024` multipliers

So the factor also does not change existing behaviour ;-). 
RHofsaess: > Thanks, that looks great!
> I took the time to check the Slurm sources, and indeed:
> https://github.com/SchedMD/slurm/blob/3e7b49a16cc7403245a2d8d7bd1f2b0df3228128/src/common/slurm_protocol_defs.c#L6022
> 
> * `gb` uses 1000 multipliers
> * `g` and `gib` use `1024` multipliers
> 
> So the factor also does not change existing behaviour ;-).

Perfect, thanks. This would have been my question!
Nothing else to add from my side.

giffels: I will add additional unittests and a short comment to the code. Will ask for re-reviewing soon. Thanks!

Patch:
diff --git a/docs/source/changelog.rst b/docs/source/changelog.rst
index 0fa911c..2d008be 100644
--- a/docs/source/changelog.rst
+++ b/docs/source/changelog.rst
@@ -1,4 +1,4 @@
-.. Created by changelog.py at 2021-07-12, command
+.. Created by changelog.py at 2021-07-15, command
    '/Users/giffler/.cache/pre-commit/repor6pnmwlm/py_env-python3.9/bin/changelog docs/source/changes compile --output=docs/source/changelog.rst'
    based on the format of 'https://keepachangelog.com/'
 
@@ -6,7 +6,7 @@
 CHANGELOG
 #########
 
-[Unreleased] - 2021-07-12
+[Unreleased] - 2021-07-15
 =========================
 
 Added
diff --git a/tardis/adapters/sites/slurm.py b/tardis/adapters/sites/slurm.py
index 72b4f69..96154c2 100644
--- a/tardis/adapters/sites/slurm.py
+++ b/tardis/adapters/sites/slurm.py
@@ -9,8 +9,7 @@ from ...utilities.attributedict import AttributeDict
 from ...utilities.attributedict import convert_to_attribute_dict
 from ...utilities.executors.shellexecutor import ShellExecutor
 from ...utilities.asynccachemap import AsyncCacheMap
-from ...utilities.utils import csv_parser
-from ...utilities.utils import submit_cmd_option_formatter
+from ...utilities.utils import convert_to, csv_parser, submit_cmd_option_formatter
 
 from asyncio import TimeoutError
 from contextlib import contextmanager
@@ -174,7 +173,7 @@ class SlurmAdapter(SiteAdapter):
         walltime = self.machine_type_configuration.Walltime
 
         drone_environment = ",".join(
-            f"TardisDrone{key}={value}"
+            f"TardisDrone{key}={convert_to(value, int, value)}"
             for key, value in self.drone_environment(
                 drone_uuid, machine_meta_data_translation_mapping
             ).items()
@@ -190,7 +189,12 @@ class SlurmAdapter(SiteAdapter):
             ),
             long=AttributeDict(
                 **sbatch_options.get("long", AttributeDict()),
-                mem=f"{self.machine_meta_data.Memory}gb",
+                # slurm does not accept floating point variables for memory,
+                # therefore use internally megabytes and convert it to an integer
+                # to allow for request i.e. 2.5 GB in the machine meta data. According
+                # to http://cern.ch/go/x7p8 SLURM is using factors of 1024 to convert
+                # between memory units
+                mem=f"{int(self.machine_meta_data.Memory * 1024)}mb",
                 export=f"SLURM_Walltime={walltime},{drone_environment}",
             ),
         )
diff --git a/tardis/utilities/utils.py b/tardis/utilities/utils.py
index 20d21ea..62f5216 100644
--- a/tardis/utilities/utils.py
+++ b/tardis/utilities/utils.py
@@ -4,7 +4,8 @@ from ..exceptions.executorexceptions import CommandExecutionFailure
 from ..interfaces.executor import Executor
 
 from io import StringIO
-from typing import List, Tuple
+from typing import Any, Callable, List, TypeVar, Tuple
+
 
 import csv
 import logging
@@ -131,3 +132,16 @@ def submit_cmd_option_formatter(options: AttributeDict) -> str:
             option_string += tmp_option_string
 
     return option_string.strip()
+
+
+T = TypeVar("T")
+sentinel = object()
+
+
+def convert_to(
+    value: Any, convert_to_type: Callable[[Any], T], default: Any = sentinel
+) -> T:
+    try:
+        return convert_to_type(value)
+    except ValueError:
+        return default


Test patch:
diff --git a/.github/workflows/deployment-tests.yml b/.github/workflows/deployment-tests.yml
index 8d7e215..c8617d0 100644
--- a/.github/workflows/deployment-tests.yml
+++ b/.github/workflows/deployment-tests.yml
@@ -43,8 +43,8 @@ jobs:
       - name: Install dependencies on ${{ matrix.os_container }}
         run: |
           python3 -m pip install --upgrade pip
-          pip install .[contrib]
-          pip install coverage codecov
+          python3 -m pip install .[contrib]
+          python3 -m pip install coverage codecov
       - name: Test with unittest on ${{ matrix.os_container }}
         run: |
           coverage run -m unittest -v
@@ -58,8 +58,8 @@ jobs:
       - name: Install dependencies on ${{ matrix.platform }}
         run: |
           python3 -m pip install --upgrade pip
-          pip install .[contrib]
-          pip install coverage codecov
+          python3 -m pip install .[contrib]
+          python3 -m pip install coverage codecov
       - name: Test with unittest on ${{ matrix.platform }}
         run: |
           coverage run -m unittest -v
diff --git a/tests/adapters_t/sites_t/test_slurm.py b/tests/adapters_t/sites_t/test_slurm.py
index 63bdc8b..7a979cf 100644
--- a/tests/adapters_t/sites_t/test_slurm.py
+++ b/tests/adapters_t/sites_t/test_slurm.py
@@ -157,18 +157,20 @@ class TestSlurmAdapter(TestCase):
         expected_resource_attributes.update(
             created=datetime.now(), updated=datetime.now()
         )
-        returned_resource_attributes = run_async(
-            self.slurm_adapter.deploy_resource,
-            resource_attributes=AttributeDict(
-                machine_type="test2large",
-                site_name="TestSite",
-                obs_machine_meta_data_translation_mapping=AttributeDict(
-                    Cores=1,
-                    Memory=1000,
-                    Disk=1000,
-                ),
-                drone_uuid="testsite-1390065",
+
+        resource_attributes = AttributeDict(
+            machine_type="test2large",
+            site_name="TestSite",
+            obs_machine_meta_data_translation_mapping=AttributeDict(
+                Cores=1,
+                Memory=1024,
+                Disk=1024,
             ),
+            drone_uuid="testsite-1390065",
+        )
+
+        returned_resource_attributes = run_async(
+            self.slurm_adapter.deploy_resource, resource_attributes
         )
 
         self.assertLess(
@@ -183,7 +185,27 @@ class TestSlurmAdapter(TestCase):
         )
 
         self.mock_executor.return_value.run_command.assert_called_with(
-            "sbatch -p normal -N 1 -n 20 -t 60 --mem=62gb --export=SLURM_Walltime=60,TardisDroneCores=20,TardisDroneMemory=62000,TardisDroneDisk=100000,TardisDroneUuid=testsite-1390065 pilot.sh"  # noqa: B950
+            "sbatch -p normal -N 1 -n 20 -t 60 --mem=63488mb --export=SLURM_Walltime=60,TardisDroneCores=20,TardisDroneMemory=63488,TardisDroneDisk=102400,TardisDroneUuid=testsite-1390065 pilot.sh"  # noqa: B950
+        )
+
+        self.mock_executor.reset_mock()
+
+        self.test_site_config.MachineMetaData.test2large.Memory = 2.5
+
+        run_async(self.slurm_adapter.deploy_resource, resource_attributes)
+
+        self.mock_executor.return_value.run_command.assert_called_with(
+            "sbatch -p normal -N 1 -n 20 -t 60 --mem=2560mb --export=SLURM_Walltime=60,TardisDroneCores=20,TardisDroneMemory=2560,TardisDroneDisk=102400,TardisDroneUuid=testsite-1390065 pilot.sh"  # noqa: B950
+        )
+
+        self.mock_executor.reset_mock()
+
+        self.test_site_config.MachineMetaData.test2large.Memory = 2.546372129
+
+        run_async(self.slurm_adapter.deploy_resource, resource_attributes)
+
+        self.mock_executor.return_value.run_command.assert_called_with(
+            "sbatch -p normal -N 1 -n 20 -t 60 --mem=2607mb --export=SLURM_Walltime=60,TardisDroneCores=20,TardisDroneMemory=2607,TardisDroneDisk=102400,TardisDroneUuid=testsite-1390065 pilot.sh"  # noqa: B950
         )
 
     @mock_executor_run_command(TEST_DEPLOY_RESOURCE_RESPONSE)
@@ -209,7 +231,7 @@ class TestSlurmAdapter(TestCase):
         )
 
         self.mock_executor.return_value.run_command.assert_called_with(
-            "sbatch -p normal -N 1 -n 20 -t 60 --gres=tmp:1G --mem=62gb --export=SLURM_Walltime=60,TardisDroneCores=20,TardisDroneMemory=62000,TardisDroneDisk=100000,TardisDroneUuid=testsite-1390065 pilot.sh"  # noqa: B950
+            "sbatch -p normal -N 1 -n 20 -t 60 --gres=tmp:1G --mem=63488mb --export=SLURM_Walltime=60,TardisDroneCores=20,TardisDroneMemory=62000,TardisDroneDisk=100000,TardisDroneUuid=testsite-1390065 pilot.sh"  # noqa: B950
         )
 
     def test_machine_meta_data(self):