Merge pull request #383 from JosuaCarl/fix-session-memory

Fix floating point error and refactor Bytes for value types

Author: JosuaCarl

CHANGELOG.md

@@ -2,11 +2,13 @@
 ## Bug Fixes:
 - CPU power model now applied correctly
 - Unintended report value removal
+- Fixed floating point error for numerical conversion division
 
 ## Misc:
 - Improved workflow reporting form extension/CLI by deriving and injecting workflow metadata from the provided trace file
 - Added full integration test
 - Adapted to Nextflow 26
+- Reworked `Bytes`, such that it can take binary and decimal-based values
 
 ## Features:
 - Transformation of data file to provenance file with schema.org / bioschemas.org type annotation in JSON-LD data format

docs/usage/FAQ.md

@@ -31,4 +31,17 @@
     - Set the TDP via `powerdrawCpuDefault = <TDP per core>` and then ignore the warning with `ignoreCpuModel = true`.
 
     For more information see our documentation on [power draw parameters](./parameters.md#hardware-power-draw). You can additionally
-    report the model with the ["Missing chip" GitHub issue](https://github.com/nextflow-io/nf-co2footprint/issues/new?template=missing_chip.yaml).
+    report the model with the ["Missing chip" GitHub issue](https://github.com/nextflow-io/nf-co2footprint/issues/new?template=missing_chip.yaml).
+
+
+<a id="differing-memory"></a>
+??? faq "Differing memory values"
+
+    The conversion of `Byte`-type units, such as `memory` is done in accordance with [SI standard, decimal-based naming](https://en.wikipedia.org/wiki/Metric_prefix#List_of_SI_prefixes), rather than [Nextflows binary-based system](https://en.wikipedia.org/wiki/Byte#Multiple-byte_units).
+    In essence, we use: 1 GB = 1 000 000 000 bytes (1 * 1000^3), while Nextflow uses: 1 GB = 1 073 741 824 bytes (1 * 1024^3), which should be 1 GiB (Gibibyte).
+    
+    Example: <br>
+    &emsp;    -> You request `process.memory = 12 GB` <br>
+    &emsp;    -> Nextflow converts it to 12884901888 bytes and passes the restriction onto the executor <br>
+    &emsp;&emsp;        => 12.88 GB = 12 GiB are allocated for the process <br>
+    &emsp;    -> The plugin uses the the former to proceed with calculations and reporting, while Nextflow (incorrectly) reports using the latter <br>

src/main/nextflow/co2footprint/CO2FootprintCalculator.groovy

@@ -85,8 +85,8 @@ class CO2FootprintCalculator {
 
         /* ===== Memory Information ===== */
 
-        final Long requestedMemory = trace.get('memory') as Long        // [bytes]
-        final Long maxRequiredMemory = trace.get('peak_rss') as Long    // [bytes]
+        final BigInteger requestedMemory = trace.get('memory') as BigInteger        // [bytes]
+        final BigInteger maxRequiredMemory = trace.get('peak_rss') as BigInteger    // [bytes]
 
         // Assign the final memory value
         final BigDecimal memory
@@ -120,7 +120,7 @@ class CO2FootprintCalculator {
         final BigDecimal ci = timeCiRecords.getCi(trace)
 
         // Personal energy mix based carbon intensity
-        final Double ciMarket = config.ciMarket
+        final BigDecimal ciMarket = config.ciMarket
 
 
         /* ===== Energy & Emission Calculation ===== */
@@ -147,9 +147,9 @@ class CO2FootprintCalculator {
             co2eMarket,
             ci,
             cpuUsage,
-            memory as Long,
+            memory,
             runtime_h,
-            numberOfCores as Integer,
+            numberOfCores,
             powerdrawPerCore,
             config.ignoreCpuModel ? 'Custom value' : cpuModel,
             rawEnergyProcessor,

src/main/nextflow/co2footprint/Metrics/Bytes.groovy

@@ -1,24 +1,28 @@
 package nextflow.co2footprint.Metrics
 
+/**
+ * Class to handle Byte metrics
+ */
 class Bytes extends Quantity {
     /**
      * Creator of Bytes, a Quantity which represents a number in bytes 
      *
      * @param value         The numeric value.
-     * @param scale         The unit scale (e.g. '', 'K', 'M'). Defaults to ''.
+     * @param scale         The unit scale (e.g. '', 'K', 'M' or 'Ki, 'Gi',...). Defaults to ''.
      * @param type          The datatype label. Defaults to 'Bytes'.
      * @param description   Optional human-readable description of the value.
      */
     Bytes(Object value, String scale='', String type='Bytes', String description = null) {
         super(value, scale, 'B', type, description)
-        scalingFactor = 1024
+        scalingLists.add(['', 'Ki', 'Mi', 'Gi', 'Ti', 'Pi', 'Ei', 'Zi', 'Yi', 'Ri', 'Qi'])
+        scalingFactors.add(1024)
     }
 
     /**
      * Creates a {@link Metric} or a {@link Bytes}, if the value is numerical.
      *
      * @param value         The raw value.
-     * @param scale         The unit scale (e.g. '', 'K', 'M'). Defaults to ''.
+     * @param scale         The unit scale (e.g. '', 'K', 'M' or 'Ki, 'Gi',...). Defaults to ''.
      * @param type          The type label for the metric. Defaults to 'Bytes'.
      * @param description   Optional human-readable description of the metric.
      * @return              A Bytes or Metric object depending on the input value.
@@ -31,5 +35,4 @@ class Bytes extends Quantity {
             return new Metric(value, type, "${scale}B", description)
         }
     }
-
 }

src/main/nextflow/co2footprint/Metrics/Quantity.groovy

@@ -12,7 +12,10 @@ class Quantity extends Metric<BigDecimal> {
     String scale
     boolean integerType
     String separator = ' '
-    int scalingFactor = 1000
+
+    // Units: pico, nano, micro, milli, 0, Kilo, Mega, Giga, Tera, Peta, Exa (decimal 1000 scale)
+    List<List<String>> scalingLists = [['p', 'n', 'u', 'm', '', 'k', 'M', 'G', 'T', 'P', 'E']]
+    List<Integer> scalingFactors = [1000]
 
     /**
      * Creator of a Quantity, combining the tracking and reporting of a number, associated with a unit.
@@ -83,15 +86,16 @@ class Quantity extends Metric<BigDecimal> {
      * Converts a numeric value to the closest or given scale with SI prefixes.
      * Scales the value up or down by a given factor and adjusts the scale prefix accordingly.
      *
+     * @param value The value that is to be converted
+     * @param currentScale The current scale
+     * @param scales The scales that are available
+     * @param scalingFactor The scaling factor between the units
      * @param targetScale The scale that should be converted to (e.g. G), default of null (optional)
-     * @return Converted quantity with appropriate scale
+     * @return Converted value
      */
-    Quantity scale(String targetScale=null) {
+    static ScaledValue applyScale(BigDecimal value, String currentScale, List<String> scales, int scalingFactor, String targetScale=null) {
         if (value) {
-
-            final List<String> scales = ['p', 'n', 'u', 'm', '', 'k', 'M', 'G', 'T', 'P', 'E']
-            // Units: pico, nano, micro, milli, 0, Kilo, Mega, Giga, Tera, Peta, Exa
-            int scaleIndex = getIdx(scale, scales)
+            int scaleIndex = getIdx(currentScale, scales)
 
             int targetScaleIndex
             int difference
@@ -108,11 +112,45 @@ class Quantity extends Metric<BigDecimal> {
                 ))
                 targetScaleIndex = scaleIndex + difference
             }
-            value = (value / scalingFactor**difference) as BigDecimal
+            value = value.divide(scalingFactor**difference)
             targetScale = scales[targetScaleIndex]
         }
 
-        scale = targetScale
+        return new ScaledValue(value, targetScale)
+    }
+
+    /**
+     * Converts a numeric value to the closest or given scale with SI prefixes.
+     * Scales the value up or down by a given factor and adjusts the scale prefix accordingly.
+     *
+     * @param targetScale The scale that should be converted to (e.g. G), default of null (optional)
+     * @return Converted quantity with appropriate scale
+     */
+    Quantity scale(String targetScale=null) {
+        // Search for scales in the available lists
+        int scalePos = scalingLists.findIndexOf { List<String> scalingList -> scalingList.contains(scale) }
+        int targetScalePos = scalingLists.findIndexOf { List<String> scalingList -> scalingList.contains(targetScale) }
+        
+        // Define current scaling list and factor
+        List<String> scalingList = scalingLists[scalePos]
+        Integer scalingFactor = scalingFactors[scalePos]
+        
+        ScaledValue scaledValue
+        if(targetScale == null || scalePos == targetScalePos) {
+            scaledValue = applyScale(value, scale, scalingList, scalingFactor, targetScale)
+        }
+        else {
+            // Define target scaling list and factor
+            List<String> targetScalingList = scalingLists[targetScalePos]
+            Integer targetScalingFactor  = scalingFactors[targetScalePos]
+            
+            // Convert to base value and then to target scale
+            scaledValue = applyScale(value, scale, scalingList, scalingFactor, '')
+            scaledValue = applyScale(scaledValue.value, scaledValue.scale, targetScalingList, targetScalingFactor, targetScale)
+        }
+
+        value = scaledValue.value 
+        scale = scaledValue.scale
         return this
     }
 
@@ -184,4 +222,17 @@ class Quantity extends Metric<BigDecimal> {
         map['value'] = returnValue()
         return map
     }
+
+    /**
+     * Container class for a scaled value
+     */
+    static class ScaledValue {
+        BigDecimal value
+        String scale
+        
+        ScaledValue(BigDecimal value, String scale) {
+            this.value = value
+            this.scale = scale
+        }
+    }
 }

src/main/nextflow/co2footprint/Parsers/TraceFileParser.groovy

@@ -43,7 +43,7 @@ class TraceFileParser {
                     value = switch (TraceRecord.FIELDS.get(key)) {
                         case 'mem' -> if (value.endsWith('B')) {
                             List<String> split = value.split(' ')
-                            Metric<BigDecimal> bytes = Bytes.of(split[0].toDouble(), split[1].dropRight(1)).scale('')
+                            Metric<BigDecimal> bytes = Bytes.of(split[0] as BigDecimal, split[1].dropRight(1)).scale('')
                             bytes.value.toLong()
                         } else {
                             value.toLong()

src/main/nextflow/co2footprint/Recorders/SessionTraceRecorder.groovy

@@ -131,12 +131,14 @@ class SessionTraceRecorder {
         )
 
         if (samples) {
+            List<Long> rss = samples.collect({ MemorySample sample -> sample.rssBytes})
+            List<Long> vmem = samples.collect({ MemorySample sample -> sample.virtualMemoryBytes})
             sessionRecord.putAll(
                     [
-                            rss:            samples.collect({ MemorySample sample -> sample.rssBytes}).average() as Long,
-                            vmem:           samples.collect({ MemorySample sample -> sample.virtualMemoryBytes}).average() as Long,
-                            peak_rss:       samples.collect({ MemorySample sample -> sample.rssBytes}).max() as Long,
-                            peak_vmem:      samples.collect({ MemorySample sample -> sample.virtualMemoryBytes}).max() as Long,
+                            rss:            rss.average(),
+                            vmem:           vmem.average(),
+                            peak_rss:       rss.max(),
+                            peak_vmem:      vmem.max(),
                     ]
             )
         }

src/main/nextflow/co2footprint/Records/CO2Record.groovy

@@ -80,9 +80,9 @@ class CO2Record extends TraceRecord {
     * @param rawEnergyMemory    Memory-specific energy consumed by the task (kWh)
     */
     CO2Record(
-        TraceRecord traceRecord, Double energy, Double co2e, Double co2eMarket, Double ci,
-        Double cpuUsage, Long memory, Double time,  Integer cpus, Double powerdrawCPU,
-        String cpu_model, Double rawEnergyProcessor, Double rawEnergyMemory
+        TraceRecord traceRecord, BigDecimal energy, BigDecimal co2e, BigDecimal co2eMarket, BigDecimal ci,
+        BigDecimal cpuUsage, BigDecimal memory, BigDecimal time, Integer cpus, BigDecimal powerdrawCPU,
+        String cpu_model, BigDecimal rawEnergyProcessor, BigDecimal rawEnergyMemory
     ) {
         // Add trace Record values
         traceKeys = traceRecord.store.keySet() as List<String>
@@ -240,7 +240,7 @@ class CO2Record extends TraceRecord {
             case 'carbon_intensity' -> new Quantity(value, '', 'gCO₂e/kWh').toReadable()
             case 'powerdraw_cpu' ->  new Quantity(value, '', 'W').toReadable()
             case '%cpu' ->  new Percentage(value).toReadable()
-            case 'memory' ->  new Bytes(value, 'G', 'B').toReadable()
+            case 'memory' ->  new Bytes(value, 'G').toReadable()
             case 'raw_energy_processor' ->  new Quantity(value, 'k', 'Wh').toReadable()
             case 'raw_energy_memory' ->  new Quantity(value, 'k', 'Wh').toReadable()
             default -> getFmtStr(key)

src/test/nextflow/co2footprint/CO2FootprintCalculatorTest.groovy

@@ -40,7 +40,7 @@ class CO2FootprintCalculatorTest extends Specification{
         traceRecord.cpus = 2
         traceRecord.cpu_model = cpuModel
         traceRecord.'%cpu' = 100.0
-        traceRecord.memory = (7 as Long) * (1024**3 as Long)
+        traceRecord.memory = (7 as Long) * (1000**3 as Long)
 
         CO2FootprintConfig config = new CO2FootprintConfig(configMap, tdpDataMatrix, ciDataMatrix, [:])
         CO2FootprintCalculator co2FootprintComputer = new CO2FootprintCalculator(tdpDataMatrix, config)
@@ -101,28 +101,28 @@ class CO2FootprintCalculatorTest extends Specification{
 
         when:
         // Try to compute the CO2 footprint, catching any exceptions
-        def result = null
-        def caught = null
+        CO2Record result = null
+        Exception caught = null
         try {
             result = co2FootprintComputer.computeTaskCO2footprint(traceRecord, timeCiRecordCollector)
-        } catch (Exception e) {
+        } catch (MissingValueException e) {
             caught = e
         }
 
         then:
         // If we expect an exception, assert it was thrown
-        if (expectException) {
-            assert caught instanceof MissingValueException
+        if (caught) {
+                assert expectException
         } else {
             // Otherwise, check that the computed memory matches the expected value (in GB)
             assert result.store.memory == expectedMemory
         }
 
         where:
         memory             | peak_rss           | expectException | expectedMemory
-        8L*1024**3         | 4L*1024**3         | false           | 8L              // requested memory used
-        null               | 4L*1024**3         | false           | 4L              // peak_rss used (requested null)
-        4L*1024**3         | null               | false           | 4L              // requested used (required null)
+        8L*1000**3         | 4L*1000**3         | false           | 8L              // requested memory used
+        null               | 4L*1000**3         | false           | 4L              // peak_rss used (requested null)
+        4L*1000**3         | null               | false           | 4L              // requested used (required null)
         null               | null               | true            | null            // throws error (both null)
     }
 
@@ -134,7 +134,7 @@ class CO2FootprintCalculatorTest extends Specification{
         traceRecord.cpus = cpus
         traceRecord.cpu_model = "Some model"
         traceRecord.'%cpu' = pCpu
-        traceRecord.memory = (7 as Long) * (1024**3 as Long)
+        traceRecord.memory = (7 as Long) * (1000**3 as Long)
 
         CO2FootprintConfig config = new CO2FootprintConfig([:], tdpDataMatrix, ciDataMatrix, [:])
         CO2FootprintCalculator co2FootprintComputer = new CO2FootprintCalculator(tdpDataMatrix, config)

src/test/nextflow/co2footprint/CO2FootprintObserverTest.groovy

@@ -41,7 +41,7 @@ class CO2FootprintObserverTest extends Specification{
                  'cpus': 1,
                  'cpu_model': "Unknown model",
                  '%cpu': 100.0,
-                 'memory': (7 as Long) * (1024**3 as Long), // 7 GB
+                 'memory': (7 as Long) * (1000**3 as Long), // 7 GB
                  'status': 'COMPLETED'
             ]
         )