Skip to content

[codex] add post-report benchmark artifacts - #44

Draft
jmchandonia wants to merge 1 commit into
codex/overall-benchmark-evaluationfrom
codex/post-report-results-and-conversation
Draft

[codex] add post-report benchmark artifacts#44
jmchandonia wants to merge 1 commit into
codex/overall-benchmark-evaluationfrom
codex/post-report-results-and-conversation

Conversation

@jmchandonia

Copy link
Copy Markdown
Collaborator

Summary

Adds post-report benchmark artifacts found after the overall benchmark evaluation branch was checked in:

  • local .runs result artifacts for the July 1 parallel leave-one-out batch, excluding staged raw data and generated harmonized CSVs that are too large for normal GitHub storage
  • checksum, byte-size, and line-count manifest for omitted generated CSVs
  • downloaded GitHub Actions artifact for successful run 28483156122 (eval-holdout-7)
  • raw Codex conversation JSONL plus a readable Markdown extraction so reviewers can inspect the user's instructions and Codex responses
  • explicit trace-retention note that future sub-agent runs must persist complete tool-call JSONL traces, not just action logs

Notes

The committed conversation JSONL was scanned for obvious credential patterns and token-shaped values were redacted in the committed copy. The source session file outside the repo was not modified.

Large generated CSVs were omitted because several exceed normal GitHub file-size limits, including a 394 MB combined CSV and a 116 MB holdout-23 CSV. Their SHA-256 digests and line counts are recorded in evaluations/post_report_results/parallel_batch_20260701/large_generated_csv_manifest.csv.

Validation

  • git diff --cached --check
  • scanned committed conversation JSONL for GitHub/OpenAI/Bearer/private-key patterns
  • checked that no committed artifact file exceeds 95 MB

@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @jmchandonia's task —— View job


PR Review: Add Post-Report Benchmark Artifacts

Tasks:

  • Gather context and understand the PR
  • Review Python code files
  • Review security (JSONL, credentials)
  • Review data/manifest files
  • Review documentation
  • Provide final feedback

Summary

This PR adds benchmark artifact files produced after the main evaluation report was finalized: local .runs/ result artifacts from a July 1 parallel leave-one-out batch, a downloaded GitHub Actions artifact for holdout-7, and a raw Codex session JSONL with a Markdown extraction. The overall organization is clear and the rationale is well-documented.


Code Quality

fold-01 harmonize_heldout.py (fold-01-holdout-1-2-3-6-16-27)

  • Hardcoded absolute paths (/scratch/jmc/, /h/jmc/) at the module level (FOLD, RAW) make the script non-portable and non-reproducible on any other machine. fold-02's script has the same issue. If these scripts are ever re-run or validated by a different team member, they will fail silently or require manual editing.
    • evaluations/post_report_results/parallel_batch_20260701/runs/fold-01-holdout-1-2-3-6-16-27/agent_outputs/harmonize_heldout.py:8-12
  • Timestamp hardcoding in log(): The log() function writes a fixed string "2026-06-30T00:00:00-07:00" as the timestamp, not the actual wall-clock time. This misleads any tooling that parses the log for timing data.
    • fold-01 harmonize_heldout.py:34
  • finalize_long() uses 'first' aggregation for competing measurements: When rows with the same keys have multiple non-null VWC or water-potential values, only the first is kept without a warning. This silently discards data that might be valid (e.g., from different replicate sensors collapsed by the groupby).
    • fold-01 harmonize_heldout.py:84-88

fold-02 harmonize_heldout.py — Manual UTM-to-lat/lon conversion

  • The script implements the full Transverse Mercator inverse formula by hand (lines 80–117). common.py already provides utm32613_to_latlon() via pyproj. The fold-02 script re-implements a nearly identical projection (UTM zone 13N) without reusing the shared helper, increasing maintenance burden and introducing a risk of subtle formula errors. The common.py version using pyproj is the canonical, well-tested approach.
    • fold-02 harmonize_heldout.py:80-117
  • iso_utc() formats as %Y-%m-%dT%H:%M:%SZ but the timezone-aware datetime already carries UTC info. The result is a naive-appearing string; if mixed with tz-aware datetimes downstream, this could cause comparison issues.
    • fold-02 harmonize_heldout.py:75-76

github_actions harmonize_dataset_07.py

  • __locations is a private-name variable (double-underscore prefix) but is used as a plain accumulator list, not a Python name-mangled class attribute. The naming is misleading; it should just be locations.
    • output/harmonize_dataset_07.py:57
  • x.loc[x["interval_min"] < 0, "interval_min"] = np.nan: The negative-interval guard is correct, but it silently discards intervals that arise from out-of-order timestamps after sorting. Since the sort by ["site_id", "depth_m", "datetime_UTC"] should already handle this, the negative values would only appear due to duplicate timestamps or parsing failures. Adding a brief comment would help future maintainers.
    • output/harmonize_dataset_07.py:85
  • The standalone runner at the bottom (lines 122–151) calls pd.concat(result.locations, ...) without guarding against an empty locations list, which would raise a ValueError. The harmonize() function always appends loc to __locations, so in practice this is safe, but defensive code would be preferable.
    • output/harmonize_dataset_07.py:151

common.pyharmonize_locations()

  • O(n²) combinations() loop over all location pairs. For small datasets this is fine, but the PR's report mentions 27+ datasets with potentially many sites. If location counts grow, this quadratic scan will become a bottleneck.
    • common.py:270
  • uuid.uuid4() generates non-deterministic UUIDs on every run, meaning two runs of the same data produce different UUIDs. This makes reproducibility checks (e.g., diff of output CSVs) harder. A deterministic UUID based on a hash of the canonical coordinates or site_id would be more suitable for a benchmark.
    • common.py:289
  • harmonize_locations() uses import statements inside the function body (import re, import uuid, from itertools import combinations). These should be at the module level per PEP 8.
    • common.py:165-167

Security Concerns

Conversation JSONL — credential redaction is incomplete / inconsistent

The PR description states token-shaped values were redacted, and the README confirms this. The REDACTED grep shows 4 occurrences. However, reviewing the JSONL directly reveals:

  1. Encrypted reasoning blobs are committed verbatim — entries like "encrypted_content": "gAAAAABqRD..." are present throughout the file. These are OpenAI's encrypted reasoning outputs. While they are ciphertext (not plaintext credentials), they constitute opaque model internals from a paid API session. It is not possible to verify from this repository what they contain without the decryption key. The PR should acknowledge this explicitly or omit the reasoning entries.

  2. gh auth status output with Token: <REDACTED_GITHUB_TOKEN> appears in at least two function_call_output entries (search: REDACTED_GITHUB_TOKEN). The token value is already redacted (replaced with the tag), which is correct. However, the /h/jmc/.config/gh/hosts.yml path and the jmchandonia GitHub username are exposed in the session log — this is low-risk but is PII that reviewers should be aware of.

  3. Approved prefix rules list in the session header (line 1 of the JSONL) is very long and includes dozens of previously approved escalation prefixes (e.g. ["curl", "-sS", "-u"], ["git", "tag", "-a"]). These are not secrets, but they expose the user's full Codex privilege configuration and project paths (/h/jmc/src/BERIL-research-observatory/, /h/jmc/src/CORAL/, etc.). If this was intentional (for reproducibility), it should be noted in the README.

Recommendation: Consider stripping the "encrypted_content" fields and the session_meta/permissions_instructions section from the committed JSONL, since those don't contribute to reviewer understanding of the agent's data harmonization decisions. The readable Markdown extraction in conversation_019f1a4b-79ea-7070-bee5-609ceccdba08.md already serves the purpose of showing what the user instructed and what the agent responded.


Potential Bugs / Issues

  • fold-03 write-location violation: The AGENT_ACTION_LOG.failed-write-location.md documents that the agent attempted to write output files outside its sandbox (/scratch/jmc/data-harmonization-eval/agent_outputs/ instead of the fold-local agent_outputs/). The .failed-write-location.md file is committed alongside the recovered .md, which is good for traceability. However, the fold-03 agent_outputs/harmonize_heldout.py in the repo may be the one written to the wrong location and then manually rescued — the provenance is unclear. The README or a per-fold note should clarify this.

  • large_generated_csv_manifest.csv SHA-256 anomaly: The combined heldout_harmonized.csv for fold-01 is listed as 3,584,772 lines in the manifest but the verification_summary.json reports row_count: 3,584,771. The off-by-one is almost certainly the header row, but it would be cleaner to document whether line_count includes the header.

    • large_generated_csv_manifest.csv:8 vs verification_summary.json:17
  • fold-02 missing verification_summary.json: Every other fold has a verification_summary.json but fold-02 does not (it's absent from the file_inventory.csv). This is inconsistent and the reason should be documented.


Performance Considerations

  • The harmonize_locations() function in common.py uses an O(n²) pairwise comparison (via combinations(range(n), 2)) with a Python-level function call per pair. For the current dataset sizes this is acceptable, but if the benchmark scales to hundreds of datasets with hundreds of sites each, this should be replaced with spatial indexing (e.g., a k-d tree or BallTree) for the coordinate matching step.

Test Coverage

  • There are no automated tests for the harmonization scripts committed in this PR. Given that these are benchmark artifacts (run outputs, not production code), that is understandable. However, at minimum, the _inspect.py script in the holdout-7 artifact could be made runnable as a smoke test. Currently it hardcodes /home/runner/ess-dive_wfsfa_soil_datasets/ which will not exist on CI.
    • eval-holdout-7/_inspect.py:2

Minor / Documentation

  • evaluations/post_report_results/README.md:3 — The creation timestamp (2026-08-11T17:43:41+00:00) is auto-generated, but the parallel_batch_20260701/ subdirectory README is absent. A brief per-batch README explaining what the folds represent would help new reviewers.
  • The ISSUE_42_PARALLEL_BATCH_REPORT.md is a useful summary, but it references issue First parallel leave-one-out batch (18 agents): quality results + isolation/audit findings #42 as a URL in markdown without linking it. Consider using [#42](https://github.com/bioepic-data/data-harmonization-eval/issues/42) syntax for navigability.

Overall Assessment

The artifact organization and manifesting strategy (large CSV manifest, file inventory, fold structure) are well-designed. The code quality is typical of agent-generated scripts — functional but with portability and reproducibility gaps that are expected in a benchmark evaluation context. The main issue warranting follow-up before merging is the conversation JSONL: the encrypted reasoning blobs and the session metadata (approved prefix rules, internal paths) were likely not intended to be committed verbatim and should be reviewed or stripped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant