From 2ce019bb715c8d503c5b78d1affd72a2074ebd20 Mon Sep 17 00:00:00 2001 From: rasmusjen Date: Mon, 24 Aug 2026 21:18:53 +0200 Subject: [PATCH 1/5] chore: stop tracking config/config.yaml The file holds machine-specific absolute paths and was tracked despite .gitignore's `config/*` intent, so every local path change churned in the diff. Untracked with `git rm --cached`; the file stays on disk and the default `--config` path is unchanged, so no invocation changes. Only config/config.yaml.example is version-controlled now, which means a fresh clone must copy it before first use. README and CLAUDE.md say so explicitly. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018hHYmK3AS1NjyeSJiJK4uj --- CLAUDE.md | 4 ++- README.md | 3 ++ config/config.yaml | 85 ---------------------------------------------- 3 files changed, 6 insertions(+), 86 deletions(-) delete mode 100644 config/config.yaml diff --git a/CLAUDE.md b/CLAUDE.md index e9d6707..4b90c41 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -51,7 +51,9 @@ pre-commit run --all-files `D:/L0_raw/`, or `D:/L1_processed/`. These hold irreplaceable raw and processed field data. - `config/config.yaml` is the user's live working config with - machine-specific paths. Don't commit changes to it. + machine-specific paths. It is untracked and gitignored — never re-add it + with `git add -f`. Edit `config/config.yaml.example` instead when a change + belongs in the repo. ## Code quality diff --git a/README.md b/README.md index a62cf96..edbac21 100644 --- a/README.md +++ b/README.md @@ -62,6 +62,9 @@ cp config/config.yaml.example config/config.yaml # Edit config/config.yaml with your paths and settings ``` +This step is required: `config/config.yaml` is gitignored (it holds +machine-specific paths), so a fresh clone does not contain one. + **Key configuration items:** - Set `eddypro_executable` to your EddyPro installation path diff --git a/config/config.yaml b/config/config.yaml deleted file mode 100644 index 1c00527..0000000 --- a/config/config.yaml +++ /dev/null @@ -1,85 +0,0 @@ -# Configuration for EddyPro processing job - -# Specify the path to the EddyPro executable -eddypro_executable: "C:/Program Files/LI-COR/EddyPro-7.0.9/bin/eddypro_rp.exe" - -# Specify the site ID you want to process -site_id: GL-ZaF -#site_id: GL-ZaH -#site_id: GL-Dsk -#site_id: GL-NuF -#site_id: GL-NuH - -# List of years you want to process for this site -years_to_process: - - #- 2000 - #- 2001 - #- 2002 - #- 2003 - #- 2004 - #- 2005 - #- 2016 - #- 2007 - #- 2008 - #- 2009 - #- 2010 - #- 2011 - #- 2012 - #- 2013 - #- 2014 - #- 2015 - #- 2016 - #- 2017 - #- 2018 - #- 2019 - #- 2020 - #- 2021 - #- 2022 - #- 2023 - #- 2024 - - 2025 -# Input directory pattern for each year and site -# Use `{year}` and `{site_id}` as placeholders -#input_dir_pattern: "C:/Users/au710242/Code/Python/eddypro_batch_processor/data/raw/{site_id}/{year}" - -input_dir_pattern: "D:/L0_raw/{site_id}/{year}/ec/rflux_csv" - -# Output directory pattern for each year and site -# Use `{year}` and `{site_id}` as placeholders -#output_dir_pattern: "C:/Users/au710242/Code/Python/eddypro_batch_processor/data/processed/{site_id}/{year}" - -output_dir_pattern: "D:/L1_processed/{site_id}/{year}/ec_rflux" - -# Path to the ECMD CSV file -#ecmd_file: "C:/Users/au710242/Code/Python/eddypro_batch_processor/data/GL-ZaF_ecmd.csv" -ecmd_file: "D:/L1_processed/{site_id}/ecmd/{site_id}_ecmd.csv" - -# Enable or disable multiprocessing -multiprocessing: False # Set to False to disable multiprocessing - -# Maximum number of CPU cores to use for multiprocessing -max_processes: 16 # Adjust this number based on your requirements - -# Control output streaming (EddyPro subprocess outputs) -stream_output: True # Set to False to keep EddyPro output quiet - -# Control logging verbosity -log_level: INFO # Options: DEBUG, INFO, WARNING, ERROR, CRITICAL - -# Optional log file path (null disables file logging) -log_file: logs/eddypro_processing.log - -# Log rotation (bytes) and backup count (0 disables rotation) -log_max_bytes: 10485760 # 10 MB -log_backup_count: 5 - -# Capture EddyPro stdout/stderr in logs -log_eddypro_output: True - -# Performance monitoring configuration -metrics_interval_seconds: 0.5 # Sampling interval for performance monitoring - -# Reporting configuration -reports_dir: null # Optional: Custom reports directory (default: {output_dir}/reports) -report_charts: plotly # Chart engine: plotly, svg, or none (default: plotly) From 0233df7838ccc68cf70c89adcda5b56ec5de50fc Mon Sep 17 00:00:00 2001 From: rasmusjen Date: Mon, 24 Aug 2026 21:19:31 +0200 Subject: [PATCH 2/5] fix(monitor,analysis): measure descendant CPU and refuse pre-v2 metrics Three related corrections to the bottleneck verdict. monitor: psutil records the CPU-times baseline on the Process *instance*, and children(recursive=True) constructs fresh objects on every sample, so each descendant only ever produced its first (always 0.0) reading. Process instances are now cached by PID and evicted when the process exits. This reproduces whenever the real work runs in a child rather than in the launched process -- a Windows venv python.exe shim, or EddyPro spawning workers -- and yielded exactly the all-zero CPU column the process-tree fix was meant to eliminate. Verified against a real subprocess: 0.0% to ~85% of one core. analysis: a metrics file with none of the canonical process-tree columns is now classified UNKNOWN with an explanation, rather than summarising absent data as "no clear bottleneck, CPU 0.0%". Every pre-v2 file on disk hits this path. analysis: default disk thresholds raised from mechanical-disk to SATA SSD scale (450/250 MB/s, 20000 IOPS), since ordinary SSD runs were being reported as disk-bound. Adds a regression test in which the launched process idles and a child does the work, so the descendant-CPU defect cannot return unnoticed on platforms where the root happens to do the work itself. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018hHYmK3AS1NjyeSJiJK4uj --- src/eddypro_batch_processor/analysis.py | 37 +++++++- src/eddypro_batch_processor/monitor.py | 24 +++++- tests/test_analysis.py | 109 ++++++++++++++++++++++-- 3 files changed, 156 insertions(+), 14 deletions(-) diff --git a/src/eddypro_batch_processor/analysis.py b/src/eddypro_batch_processor/analysis.py index d3fb400..a2ce91f 100644 --- a/src/eddypro_batch_processor/analysis.py +++ b/src/eddypro_batch_processor/analysis.py @@ -46,13 +46,24 @@ class PerformanceThresholds(TypedDict, total=False): "cpu_idle_percent": 40.0, "memory_high_percent": 85.0, "memory_moderate_percent": 70.0, - # Roughly a mechanical-disk ceiling; raise substantially for NVMe. - "disk_high_mb_per_s": 100.0, - "disk_moderate_mb_per_s": 50.0, - "disk_high_iops": 1000.0, + # Calibrated for a SATA SSD (~550 MB/s sequential), which is the common + # case for the bulk storage EddyPro reads from. Adjust for other media: + # NVMe SSD -> disk_high ~3000, moderate ~1500, iops ~200000 + # Mechanical -> disk_high ~150, moderate ~80, iops ~150 + "disk_high_mb_per_s": 450.0, + "disk_moderate_mb_per_s": 250.0, + "disk_high_iops": 20000.0, } +# Columns the process-tree monitor must emit. Their absence means the file was +# written by the pre-v2 monitor, whose figures were all 0.0 anyway (it sampled +# the shell wrapper rather than EddyPro). Reporting UNKNOWN is honest; reporting +# "no bottleneck, CPU 0.0%" from those files is not. +CANONICAL_COLUMNS = ("cpu_percent", "memory_mb", "read_mb", "write_mb") +LEGACY_COLUMNS = ("process_cpu_percent", "process_memory_rss", "process_io_read_bytes") + + @dataclass class MetricStats: """Summary statistics for one metric series.""" @@ -170,6 +181,24 @@ def analyze_rows( self, rows: list[dict[str, Any]], scenario_name: str = "baseline" ) -> ScenarioAnalysis: """Analyse already-parsed metric rows.""" + if rows and not any(c in rows[0] for c in CANONICAL_COLUMNS): + legacy = any(c in rows[0] for c in LEGACY_COLUMNS) + detail = ( + "written by a pre-v2 monitor, whose process metrics were all 0.0" + if legacy + else "missing every process-tree column" + ) + logger.warning( + f"Metrics for {scenario_name} are {detail}; cannot classify." + ) + return ScenarioAnalysis( + scenario_name=scenario_name, + sample_count=len(rows), + explanation=( + f"Unrecognised metrics schema ({detail}). Re-run with the " + f"current version to get a bottleneck verdict." + ), + ) def series(key: str) -> list[float]: return [v for v in (_to_float(r.get(key)) for r in rows) if v is not None] diff --git a/src/eddypro_batch_processor/monitor.py b/src/eddypro_batch_processor/monitor.py index 6c8c149..b03a1a8 100644 --- a/src/eddypro_batch_processor/monitor.py +++ b/src/eddypro_batch_processor/monitor.py @@ -123,6 +123,12 @@ def __init__( self._io_baseline: tuple[float, float, float, float] | None = None self._primed_pids: set[int] = set() + # psutil records the CPU-times baseline on the Process *instance*, so a + # freshly constructed object always reports 0.0. children() builds new + # objects on every call, so the instances must be cached by PID or every + # descendant reads as idle for the whole run. + self._proc_cache: dict[int, psutil.Process] = {} + # Previous sample state, for delta-based rate computation self._prev_time: float | None = None self._prev_io: tuple[float, float, float, float] | None = None @@ -285,10 +291,26 @@ def _iter_tracked(self) -> list[Any]: if not self._process: return [] try: - return [self._process, *self._process.children(recursive=True)] + children = self._process.children(recursive=True) except Exception: return [] + tracked: list[Any] = [self._process] + live_pids = {self._process.pid} + for child in children: + # setdefault keeps the first instance seen for this PID so its CPU + # baseline survives; the newly built object is discarded. + tracked.append(self._proc_cache.setdefault(child.pid, child)) + live_pids.add(child.pid) + + # Drop instances for processes that have exited. Their cumulative I/O is + # already retained separately in _io_by_pid, so nothing is lost. + for pid in self._proc_cache.keys() - live_pids: + del self._proc_cache[pid] + self._primed_pids.discard(pid) + + return tracked + def _collect_sample(self) -> dict[str, Any] | None: """ Collect a single performance sample. diff --git a/tests/test_analysis.py b/tests/test_analysis.py index 8e2767e..c7f5a02 100644 --- a/tests/test_analysis.py +++ b/tests/test_analysis.py @@ -86,8 +86,8 @@ def test_disk_throughput_bound(self, tmp_path): cpu_percent=8.0, memory_mb=300.0, system_memory_percent=35.0, - read_mb_per_s=180.0, - write_mb_per_s=20.0, + read_mb_per_s=500.0, + write_mb_per_s=100.0, ) result = BottleneckAnalyzer().analyze(csv_path) assert result.primary_bottleneck == "DISK_THROUGHPUT" @@ -103,8 +103,8 @@ def test_disk_iops_bound(self, tmp_path): system_memory_percent=30.0, read_mb_per_s=5.0, write_mb_per_s=2.0, - read_iops=3000.0, - write_iops=500.0, + read_iops=25000.0, + write_iops=5000.0, ) result = BottleneckAnalyzer().analyze(csv_path) assert result.primary_bottleneck == "DISK_IOPS" @@ -142,8 +142,8 @@ def test_cpu_takes_priority_over_disk(self, tmp_path): tmp_path / "metrics.csv", cpu_percent=96.0, system_memory_percent=40.0, - read_mb_per_s=200.0, - write_mb_per_s=200.0, + read_mb_per_s=400.0, + write_mb_per_s=400.0, ) assert BottleneckAnalyzer().analyze(csv_path).primary_bottleneck == "CPU" @@ -156,14 +156,14 @@ def test_custom_disk_threshold_changes_verdict(self, tmp_path): tmp_path / "metrics.csv", cpu_percent=8.0, system_memory_percent=30.0, - read_mb_per_s=180.0, + read_mb_per_s=600.0, ) assert BottleneckAnalyzer().analyze(csv_path).primary_bottleneck == ( "DISK_THROUGHPUT" ) - # On an NVMe drive 180 MB/s is unremarkable, so raise the ceiling. + # On an NVMe drive 600 MB/s is unremarkable, so raise the ceiling. relaxed = BottleneckAnalyzer( - {"disk_high_mb_per_s": 2000.0, "disk_moderate_mb_per_s": 1000.0} + {"disk_high_mb_per_s": 3000.0, "disk_moderate_mb_per_s": 1500.0} ) assert relaxed.analyze(csv_path).primary_bottleneck == "NONE" @@ -202,6 +202,48 @@ def test_blank_and_malformed_cells(self, tmp_path): assert result.cpu.max == 95.0 +class TestLegacySchema: + """Pre-v2 metrics files must be refused, not silently read as all-zero.""" + + LEGACY_FIELDS = [ + "process_cpu_percent", + "process_io_read_bytes", + "process_io_write_bytes", + "process_memory_rss", + "relative_time", + "system_cpu_percent", + "system_memory_percent", + "timestamp", + ] + + def _write_legacy(self, path): + with path.open("w", newline="", encoding="utf-8") as f: + writer = csv.DictWriter(f, fieldnames=self.LEGACY_FIELDS) + writer.writeheader() + for i in range(10): + row = dict.fromkeys(self.LEGACY_FIELDS, 0.0) + row["relative_time"] = float(i) + row["system_cpu_percent"] = 12.0 + row["system_memory_percent"] = 35.0 + writer.writerow(row) + return path + + def test_legacy_file_reports_unknown(self, tmp_path): + path = self._write_legacy(tmp_path / "metrics_rp.csv") + result = BottleneckAnalyzer().analyze(path) + # The old files really do contain 10 rows of zeros; saying "no + # bottleneck, CPU 0.0%" from them would be a confident lie. + assert result.primary_bottleneck == "UNKNOWN" + assert result.sample_count == 10 + assert "pre-v2" in result.explanation + + def test_current_schema_is_not_mistaken_for_legacy(self, tmp_path): + path = make_series( + tmp_path / "metrics.csv", cpu_percent=95.0, system_memory_percent=30.0 + ) + assert BottleneckAnalyzer().analyze(path).primary_bottleneck == "CPU" + + class TestDominantBottleneck: """Aggregation across several analyses.""" @@ -324,6 +366,55 @@ def test_analysis_of_real_run_is_not_unknown(self, tmp_path, burner_script): assert result.sample_count > 0 assert result.total_read_mb + result.total_write_mb > 0 + def test_cpu_is_measured_for_descendants_not_just_the_root( + self, tmp_path, burner_script + ): + """ + The root must not be the only process whose CPU is counted. + + psutil keeps the CPU-times baseline on the Process *instance*, and + ``children()`` returns freshly built objects on every call. Without an + instance cache each descendant reports 0.0 forever -- which is exactly + what happens when the launched command is a stub that re-execs into a + child (a Windows venv ``python.exe`` shim, or EddyPro spawning workers). + Here the parent deliberately idles so all the work is in the child. + """ + parent = tmp_path / "spawner.py" + parent.write_text( + textwrap.dedent(f""" + import subprocess, sys, time + p = subprocess.Popen([sys.executable, r"{burner_script}"]) + while p.poll() is None: + time.sleep(0.05) + """), + encoding="utf-8", + ) + + out_dir = tmp_path / "metrics" + rc = core.run_subprocess_with_monitoring( + command=[sys.executable, str(parent)], + working_dir=tmp_path, + stream_output=False, + metrics_interval=0.25, + output_dir=out_dir, + scenario_suffix="child", + log_output=False, + ) + assert rc == 0 + + rows = list( + csv.DictReader((out_dir / "metrics_child.csv").open(encoding="utf-8")) + ) + cpu = [ + float(r["cpu_percent_of_core"]) for r in rows if r["cpu_percent_of_core"] + ] + assert max(cpu) > 20.0, ( + "CPU was only counted for the idle root -- descendant Process " + "instances are not being cached, so their baselines never persist" + ) + procs = [int(r["num_processes"]) for r in rows if r["num_processes"]] + assert max(procs) >= 2, "the child process was never tracked" + def test_monitoring_disabled_writes_nothing(self, tmp_path, burner_script): out_dir = tmp_path / "metrics" rc = core.run_subprocess_with_monitoring( From 2a70924308adcf53bba63a4908a92af9025c5284 Mon Sep 17 00:00:00 2001 From: rasmusjen Date: Mon, 24 Aug 2026 21:19:52 +0200 Subject: [PATCH 3/5] docs: document per-medium disk thresholds and the untracked config - CONFIG.md gains a suggested-limits table for NVMe, SATA SSD and mechanical drives, plus the PowerShell one-liner to find out which medium a drive letter is on. The defaults table is updated to the new SATA-scale numbers. - CONFIG.md states that config/config.yaml is untracked and must be copied from the example on a fresh clone. - config.yaml.example and examples/multi_year_config.yaml carry the concrete threshold blocks rather than a bare mention. - CHANGELOG entries under [Unreleased] for the monitor fix, the pre-v2 metrics handling, the threshold retune and the config untracking. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018hHYmK3AS1NjyeSJiJK4uj --- CHANGELOG.md | 30 +++++++++++++++++++++++ config/config.yaml.example | 13 +++++++--- docs/CONFIG.md | 43 +++++++++++++++++++++++++++------ examples/multi_year_config.yaml | 7 ++++++ 4 files changed, 83 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cdd7181..e0d484f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,36 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **Monitor reported 0.0% CPU for every child process.** psutil records the + CPU-times baseline on the `Process` *instance*, and `children(recursive=True)` + builds fresh objects on every sample, so each descendant's first (always-zero) + reading was the only one ever taken. Process instances are now cached by PID, + so baselines persist. This mattered whenever the real work ran in a child + rather than the launched process — a Windows venv `python.exe` shim, or + EddyPro spawning workers — which produced exactly the all-zero CPU column the + process-tree fix was meant to eliminate. + +- **Pre-v2 metrics files are now reported as `UNKNOWN`** instead of being read + as a run with no bottleneck. Those files lack every process-tree column, so + the analyser was summarising absent data as `CPU 0.0%` and concluding there + was headroom to spare. + +### Changed + +- **`config/config.yaml` is no longer tracked in git.** It holds + machine-specific absolute paths that churned in every diff. The file stays + where it is and the default `--config` path is unchanged; only + `config/config.yaml.example` is version-controlled now, so a fresh clone must + copy it before first use. + +- **Default `performance_thresholds` disk limits raised to SATA SSD scale** + (`disk_high_mb_per_s` 100 → 450, `disk_moderate_mb_per_s` 50 → 250, + `disk_high_iops` 1000 → 20000). The previous values assumed a mechanical disk + and flagged ordinary SSD runs as disk-bound. `docs/CONFIG.md` now carries a + per-medium table for NVMe, SATA, and mechanical drives. + ### Added - **`--version` global CLI flag** to print the installed package version and exit. diff --git a/config/config.yaml.example b/config/config.yaml.example index 20e8bdc..87ae8be 100644 --- a/config/config.yaml.example +++ b/config/config.yaml.example @@ -59,12 +59,19 @@ monitoring_enabled: true metrics_interval_seconds: 0.5 # Optional: tune how the bottleneck analyser classifies a run. Defaults assume a -# mechanical disk; raise the disk limits substantially for NVMe. See -# docs/CONFIG.md for the full table of keys and defaults. +# SATA SSD (~550 MB/s). Set these to match the drive the input data lives on, +# otherwise the disk verdict is meaningless. See docs/CONFIG.md for every key. #performance_thresholds: # cpu_high_percent: 90 # memory_high_percent: 85 -# disk_high_mb_per_s: 100 +# # NVMe SSD: +# disk_high_mb_per_s: 3000 +# disk_moderate_mb_per_s: 1500 +# disk_high_iops: 200000 +# # Mechanical disk: +# #disk_high_mb_per_s: 150 +# #disk_moderate_mb_per_s: 80 +# #disk_high_iops: 150 # Reporting configuration reports_dir: null # Optional: Custom reports directory (default: {output_dir}/reports) diff --git a/docs/CONFIG.md b/docs/CONFIG.md index 8ba4539..1ffde30 100644 --- a/docs/CONFIG.md +++ b/docs/CONFIG.md @@ -8,6 +8,16 @@ Default location: `config/config.yaml` Override with: `--config /path/to/config.yaml` +`config/config.yaml` is **not tracked in git** — it holds machine-specific +absolute paths that differ per install. It is covered by `.gitignore`, so a +fresh clone has no `config/config.yaml` until you create one: + +```powershell +cp config/config.yaml.example config/config.yaml +``` + +Only `config/config.yaml.example` is version-controlled. + ## Configuration Schema ### Complete Example @@ -408,8 +418,11 @@ monitoring_enabled: false **Default:** see table below **Description:** Tunes how the bottleneck analyser classifies a run. The -defaults assume a mechanical disk; on NVMe storage the disk limits should be -raised substantially or every run will be reported as disk-bound. +defaults assume a **SATA SSD** (~550 MB/s sequential), which is the common case +for the bulk storage EddyPro reads from. The disk limits must match the drive +the input data actually lives on: leave NVMe limits at the SATA default and +ordinary runs are reported as disk-bound; leave mechanical-disk limits at the +SATA default and a genuinely saturated disk is never flagged at all. | Key | Default | Meaning | |-----|---------|---------| @@ -418,17 +431,33 @@ raised substantially or every run will be reported as disk-bound. | `cpu_idle_percent` | 40 | Below this, a busy disk is read as the limiting factor | | `memory_high_percent` | 85 | System memory use that counts as RED | | `memory_moderate_percent` | 70 | System memory use that counts as YELLOW | -| `disk_high_mb_per_s` | 100 | Combined read+write throughput counting as RED | -| `disk_moderate_mb_per_s` | 50 | Throughput counting as YELLOW | -| `disk_high_iops` | 1000 | Combined IOPS above which latency is the suspect | +| `disk_high_mb_per_s` | 450 | Combined read+write throughput counting as RED | +| `disk_moderate_mb_per_s` | 250 | Throughput counting as YELLOW | +| `disk_high_iops` | 20000 | Combined IOPS above which latency is the suspect | Unknown keys are ignored, so a config written for a newer version still loads. +Suggested disk limits by medium: + +| Medium | `disk_high_mb_per_s` | `disk_moderate_mb_per_s` | `disk_high_iops` | +|--------|---------------------|--------------------------|------------------| +| NVMe SSD | 3000 | 1500 | 200000 | +| SATA SSD (default) | 450 | 250 | 20000 | +| Mechanical / USB HDD | 150 | 80 | 150 | + +On Windows, check which you have with: + +```powershell +Get-PhysicalDisk | Select-Object FriendlyName, MediaType, BusType +Get-Partition | Where-Object DriveLetter | Select-Object DriveLetter, DiskNumber +``` + **Example (NVMe):** ```yaml performance_thresholds: - disk_high_mb_per_s: 2000 - disk_moderate_mb_per_s: 1000 + disk_high_mb_per_s: 3000 + disk_moderate_mb_per_s: 1500 + disk_high_iops: 200000 ``` **CLI Override:** none — config only. diff --git a/examples/multi_year_config.yaml b/examples/multi_year_config.yaml index b972b70..aca94c7 100644 --- a/examples/multi_year_config.yaml +++ b/examples/multi_year_config.yaml @@ -44,6 +44,13 @@ log_eddypro_output: true # Performance monitoring: sample CPU/memory/disk every second per year-worker. # Set monitoring_enabled: false for maximum throughput on large batches. monitoring_enabled: true + +# Bottleneck thresholds must match the drive the data lives on. These are the +# defaults, sized for a SATA SSD; see docs/CONFIG.md for NVMe and HDD values. +performance_thresholds: + disk_high_mb_per_s: 450 + disk_moderate_mb_per_s: 250 + disk_high_iops: 20000 metrics_interval_seconds: 1.0 # Reports directory set explicitly: it otherwise defaults to the FIRST From cf8aeeba6ba7ad23a6494cde294a4fed6dca07fd Mon Sep 17 00:00:00 2001 From: rasmusjen Date: Tue, 25 Aug 2026 14:28:13 +0200 Subject: [PATCH 4/5] fix(cli): show help without a config, and stop tests depending on one CI failed on all four Python versions after config/config.yaml was untracked: seven tests invoked the CLI with no --config and relied on the repo shipping that file. It passed locally only because the file is still on disk here. Two separate defects behind it. A real CLI bug: main() validated the config file before dispatching, so `eddypro-batch` with no subcommand reported "Configuration file not found" instead of printing usage. On a fresh clone that is the very first command anyone runs. The no-command case now short-circuits to print_help(), and the dispatch chain becomes a table since argparse already restricts the remaining values. A test-design flaw the untracking exposed: tests must not depend on a machine-specific file. tests/test_cli.py and tests/test_cli_functions.py now pass config/config.yaml.example explicitly via a shared EXAMPLE_CONFIG constant. Verified by moving config/config.yaml aside and running the full suite, which is the state CI actually sees: 199 passed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018hHYmK3AS1NjyeSJiJK4uj --- CHANGELOG.md | 6 ++++++ src/eddypro_batch_processor/cli.py | 28 ++++++++++++++++------------ tests/test_cli.py | 6 ++++++ tests/test_cli_functions.py | 20 ++++++++++++++++---- 4 files changed, 44 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e0d484f..1863bfe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 EddyPro spawning workers — which produced exactly the all-zero CPU column the process-tree fix was meant to eliminate. +- **`eddypro-batch` with no subcommand printed a config error instead of + help.** `main()` validated the config file before dispatching, so on a fresh + clone -- where `config/config.yaml` does not exist yet -- the first command + anyone runs failed with "Configuration file not found" rather than showing + usage. The no-command case now short-circuits to `print_help()`. + - **Pre-v2 metrics files are now reported as `UNKNOWN`** instead of being read as a run with no bottleneck. Those files lack every process-tree column, so the analyser was summarising absent data as `CPU 0.0%` and concluding there diff --git a/src/eddypro_batch_processor/cli.py b/src/eddypro_batch_processor/cli.py index 4b001e0..78e416b 100644 --- a/src/eddypro_batch_processor/cli.py +++ b/src/eddypro_batch_processor/cli.py @@ -1327,6 +1327,13 @@ def main() -> int: # Set up logging early setup_logging(args.log_level) + # With no subcommand there is nothing to configure, so help must print + # rather than a config error. On a fresh clone config/config.yaml does not + # exist yet, and a bare `eddypro-batch` is the first thing anyone runs. + if not args.command: + parser.print_help() + return 1 + # Validate config file exists if provided if hasattr(args, "config") and args.config: config_path = Path(args.config) @@ -1334,18 +1341,15 @@ def main() -> int: logging.error(f"Configuration file not found: {config_path}") return 1 - # Route to appropriate command handler - if args.command == "run": - return cmd_run(args) - elif args.command == "scenarios": - return cmd_scenarios(args) - elif args.command == "validate": - return cmd_validate(args) - elif args.command == "status": - return cmd_status(args) - else: - parser.print_help() - return 1 + # Route to appropriate command handler. argparse restricts args.command to + # these choices and the empty case returned above, so there is no fallback. + handlers = { + "run": cmd_run, + "scenarios": cmd_scenarios, + "validate": cmd_validate, + "status": cmd_status, + } + return handlers[args.command](args) if __name__ == "__main__": diff --git a/tests/test_cli.py b/tests/test_cli.py index 829d660..dbce097 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -6,6 +6,10 @@ import pytest +# config/config.yaml is untracked (machine-specific paths), so tests must never +# depend on it. The example config ships with the repo and always exists. +EXAMPLE_CONFIG = str(Path(__file__).parent.parent / "config" / "config.yaml.example") + def test_cli_help_command(): """Test that eddypro-batch --help returns zero and prints usage.""" @@ -97,6 +101,8 @@ def test_cli_scenarios_stub(): sys.executable, "-m", "eddypro_batch_processor.cli", + "--config", + EXAMPLE_CONFIG, "scenarios", "--rot-meth", "1", diff --git a/tests/test_cli_functions.py b/tests/test_cli_functions.py index cff3b7b..4a4467e 100644 --- a/tests/test_cli_functions.py +++ b/tests/test_cli_functions.py @@ -18,6 +18,11 @@ setup_logging, ) +# config/config.yaml is untracked (it holds machine-specific paths), so tests +# must never rely on it existing. The example config is version-controlled and +# is the only config guaranteed to be present in a fresh clone. +EXAMPLE_CONFIG = str(Path(__file__).parent.parent / "config" / "config.yaml.example") + def _write_ecmd_file(tmp_path: Path, site_id: str) -> Path: ecmd_path = tmp_path / "ecmd.csv" @@ -490,7 +495,7 @@ class TestMainFunction: def test_main_run_command(self): """Test main function routing to run command.""" - test_args = ["eddypro-batch", "run", "--dry-run"] + test_args = ["eddypro-batch", "--config", EXAMPLE_CONFIG, "run", "--dry-run"] with patch("sys.argv", test_args): with patch("eddypro_batch_processor.cli.cmd_run") as mock_cmd_run: @@ -503,7 +508,14 @@ def test_main_run_command(self): def test_main_scenarios_command(self): """Test main function routing to scenarios command.""" - test_args = ["eddypro-batch", "scenarios", "--rot-meth", "1"] + test_args = [ + "eddypro-batch", + "--config", + EXAMPLE_CONFIG, + "scenarios", + "--rot-meth", + "1", + ] with patch("sys.argv", test_args): with patch( @@ -518,7 +530,7 @@ def test_main_scenarios_command(self): def test_main_validate_command(self): """Test main function routing to validate command.""" - test_args = ["eddypro-batch", "validate"] + test_args = ["eddypro-batch", "--config", EXAMPLE_CONFIG, "validate"] with patch("sys.argv", test_args): with patch("eddypro_batch_processor.cli.cmd_validate") as mock_cmd_validate: @@ -531,7 +543,7 @@ def test_main_validate_command(self): def test_main_status_command(self): """Test main function routing to status command.""" - test_args = ["eddypro-batch", "status"] + test_args = ["eddypro-batch", "--config", EXAMPLE_CONFIG, "status"] with patch("sys.argv", test_args): with patch("eddypro_batch_processor.cli.cmd_status") as mock_cmd_status: From 3bbdf4e6488edb49ac69302587ad030132f72faf Mon Sep 17 00:00:00 2001 From: rasmusjen Date: Tue, 25 Aug 2026 14:32:07 +0200 Subject: [PATCH 5/5] test(monitor): assert on regression shape, not CPU magnitude CI failed at `assert 19.8 > 20.0` and `assert 15.8 > 20.0`. Both figures are real, non-zero readings -- the monitor fix works -- but a throttled 2-vCPU shared runner does not reach 20% of a core on this workload, so the threshold was measuring the runner rather than the code. The defect being guarded against produces exactly 0.0 on every sample, so magnitude is the wrong discriminator. Assert instead that CPU registered at all (> 1.0, a 15x margin below the slowest observed real reading) and, for the descendant test, that at least a third of samples recorded child CPU -- the defect records none. Verified by reintroducing the defect (appending the fresh child object rather than the cached instance): both tests still fail, at `assert 0.0 > 1.0`. The relaxed bounds lose no discriminating power. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018hHYmK3AS1NjyeSJiJK4uj --- tests/test_analysis.py | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/tests/test_analysis.py b/tests/test_analysis.py index c7f5a02..e67e707 100644 --- a/tests/test_analysis.py +++ b/tests/test_analysis.py @@ -329,8 +329,15 @@ def col(name): # The load is single-threaded, so normalised CPU is small on a many-core # box; cpu_percent_of_core is the column that must show real work. - assert max(col("cpu_percent_of_core")) > 20.0, ( - "process-tree CPU never rose above 20% of a core -- the monitor is " + # + # The failure mode being guarded against produces exactly 0.0 on every + # sample, so the discriminator is "did any real work register at all", + # not a magnitude. A throttled shared CI runner legitimately reports + # well under 20% of a core here, so asserting a specific level would + # only measure the runner. + cpu_of_core = col("cpu_percent_of_core") + assert max(cpu_of_core) > 1.0, ( + "process-tree CPU never registered any work -- the monitor is " "measuring the wrong process again" ) assert max(col("memory_mb")) > 1.0, "process memory looks like a shell" @@ -408,10 +415,24 @@ def test_cpu_is_measured_for_descendants_not_just_the_root( cpu = [ float(r["cpu_percent_of_core"]) for r in rows if r["cpu_percent_of_core"] ] - assert max(cpu) > 20.0, ( + # Without the instance cache every descendant reads 0.0 on every sample, + # and the root here is deliberately idle, so the whole column is zero. + # Assert on the *proportion* of samples that registered work rather than + # on a level: that separates 0.0-always from working-but-throttled, + # which a magnitude threshold cannot do on a shared CI runner. + assert max(cpu) > 1.0, ( "CPU was only counted for the idle root -- descendant Process " "instances are not being cached, so their baselines never persist" ) + # Drop the first sample: it is taken before the child has been primed. + # A third, not a half: on a slow runner the child can take a couple of + # samples to spawn and be primed. The defect yields zero non-zero + # samples, so any non-trivial fraction separates the two cases. + working = [c for c in cpu[1:] if c > 0.0] + assert len(working) >= len(cpu[1:]) / 3, ( + f"only {len(working)} of {len(cpu[1:])} samples registered child CPU; " + f"descendant baselines are not persisting across samples" + ) procs = [int(r["num_processes"]) for r in rows if r["num_processes"]] assert max(procs) >= 2, "the child process was never tracked"