fix(csv): strip whitespace from CSV fieldnames parsing multiple_results#115
Conversation
…results CSV headers written with spaces after commas (e.g. "model, performance, metric") cause DictReader to parse fieldnames with leading spaces, making column lookups like 'performance' not in fieldnames fail silently and report no metrics. Strip fieldnames at read time in all three execution paths: - container_runner.py: Docker local execution - kubernetes.py: K8s remote CSV parsing (fieldnames check path) - slurm.py: SLURM perf.csv parsing (also strips row keys, which were unstripped) Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes a parsing edge case where CSV headers containing spaces after delimiters (e.g. "model, performance, metric") cause csv.DictReader fieldnames to include leading whitespace, breaking downstream column lookups for multi-result performance extraction.
Changes:
- Strip CSV
DictReader.fieldnameswhen validating/readingmultiple_resultsCSVs in local container execution and Kubernetes artifact parsing. - Strip SLURM
perf.csvfieldnames (and row keys) during results collection to make header/key lookups whitespace-robust.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/madengine/execution/container_runner.py |
Strips DictReader.fieldnames before validating presence of performance in multiple_results CSV. |
src/madengine/deployment/slurm.py |
Strips perf.csv fieldnames and row keys during _collect_results_parse_perf_csv() parsing. |
src/madengine/deployment/kubernetes.py |
Strips DictReader.fieldnames before checking required columns when parsing multiple-results CSV from collected artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| # Strip whitespace from fieldnames to handle headers like "model, performance, metric" | ||
| csv_reader.fieldnames = [f.strip() for f in csv_reader.fieldnames] |
There was a problem hiding this comment.
The loop variable name f in the fieldname-stripping comprehension is easily confused with the surrounding file handle named f. Use a different name (e.g. name/col) to avoid shadowing and make the intent clearer.
| csv_reader.fieldnames = [f.strip() for f in csv_reader.fieldnames] | |
| csv_reader.fieldnames = [name.strip() for name in csv_reader.fieldnames] |
| # Strip whitespace from fieldnames to handle headers like "model, performance, metric" | ||
| csv_reader.fieldnames = [f.strip() for f in csv_reader.fieldnames] | ||
|
|
||
| # Check if 'performance' column exists | ||
| if 'performance' not in csv_reader.fieldnames: | ||
| print("Error: 'performance' column not found in multiple results file.") |
There was a problem hiding this comment.
This change fixes a real parsing edge case (headers like "model, performance, metric") but there isn't automated coverage for the multiple_results CSV validation path. Add a unit/integration test that uses a temporary CSV with spaced headers and asserts the code recognizes performance/metric columns (i.e., does not drop metrics due to header whitespace).
| with open(perf_file, "r") as f: | ||
| reader = csv.DictReader(f) | ||
| rows = list(reader) | ||
| reader.fieldnames = [f.strip() for f in (reader.fieldnames or [])] |
There was a problem hiding this comment.
The comprehension uses f as the loop variable while the file handle is also named f in the surrounding scope. Although this doesn't leak in Python 3, it’s confusing to read; rename the loop variable (e.g. col).
| reader.fieldnames = [f.strip() for f in (reader.fieldnames or [])] | |
| reader.fieldnames = [field.strip() for field in (reader.fieldnames or [])] |
| try: | ||
| with open(csv_path, "r", encoding="utf-8", errors="ignore") as f: | ||
| reader = csv_module.DictReader(f) | ||
| reader.fieldnames = [f.strip() for f in (reader.fieldnames or [])] |
There was a problem hiding this comment.
The comprehension uses f as the loop variable while the file handle is also named f in the surrounding scope. Rename the loop variable (e.g. col) to avoid shadowing and improve readability.
| reader.fieldnames = [f.strip() for f in (reader.fieldnames or [])] | |
| reader.fieldnames = [field.strip() for field in (reader.fieldnames or [])] |
| # Strip whitespace from fieldnames to handle headers like "model, performance, metric" | ||
| csv_reader.fieldnames = [f.strip() for f in csv_reader.fieldnames] |
There was a problem hiding this comment.
csv_reader.fieldnames can be None (e.g., empty CSV). The list comprehension will raise TypeError, which currently gets caught by the broad except and turns into a generic warning. Guard this like the other execution paths (e.g. (csv_reader.fieldnames or [])) and handle the empty/malformed-header case explicitly so validation fails deterministically with the intended error path.
CSV headers written with spaces after commas (e.g. "model, performance, metric")
cause DictReader to parse fieldnames with leading spaces, making column lookups
like 'performance' not in fieldnames fail silently and report no metrics.