Ci history gate#1507
Conversation
Doc-first foundation for the CI metric-history regression gate. No CI
behavior change: no gate, no DB connection at runtime, and --use-ci-history
is off by default (the harness merge is inert when MILES_CI_GATE_RECORD_DIR
is unset).
- docs/ci/03-metric-history-gate.md: the design (two normalized tables,
(test_path,backend,suite,test_file_hash) identity, two-layer rel-OR-abs
gate, nightly-only trusted writes, cleanup via trusted=false, shadow-first
rollout). No pr-test.yml edit.
- tests/ci/metric_history/: MetricHistoryStore abstraction + offline SQLite
store + versioned migrations (two-table schema, DML-only role, retention).
NeonStore deferred (NotImplementedError); requirements.txt untouched.
- miles/utils/tracking_utils/ci_history.py: CiHistoryBackend, registered as
("ci_history","use_ci_history"); per-process atomic NDJSON snapshot, safe
under Ray multi-process and resilient to the actor never calling finish().
--use-ci-history flag added (default off).
- tests/ci/ci_utils.py: per-file/per-attempt record-dir env injection into the
test Popen + post-run merge of per-process records into one per-run record.
22 offline unit tests pass (14 store + 8 collection), no network. Refs M0/M1.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…eal-CI activation; doc fix Round-2 of the CI metric-history gate; still offline with zero CI behavior change. - tests/ci/history_gate.py: evaluate_gate() — identity from CIRegistry, sha256 content hash, consumes a given merged-NDJSON record (the passed attempt), hard gate (always) + historical gate (>=1 trusted, vs mean) with rel-OR-abs tolerance, run-level trusted iff all active gates pass. Dependency-injected store; opens no DB connection, reads no wandb, writes no rows. - tests/ci/metric_reducers.py: per-metric reducers (train/grad_norm=mean_last_5, train/ppo_kl=step_zero+abs_floor, others=last) + METRIC_SPECS + ReducerError. - tests/ci/ci_register.py: register_ci_gate AST-parsed declarative spec (CiGateSpec, parse_ci_gate_specs); existing register_*_ci/RegistryVisitor untouched. - miles/utils/arguments.py: resolve_use_ci_history() — use_ci_history auto-on when MILES_CI_GATE_RECORD_DIR is set, unchanged (default off) when unset. - docs/ci/03-metric-history-gate.md: corrected PR-write-nothing contract (ordinary PR runs write no rows; nightly-marked runs are the only writer). 122 offline tests pass (36 new gate/reducer + 86 prior/regression). Refs M1/M2. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…g NeonStore + gate identity fix (M2b/M3) Round-3 of the CI metric-history gate. Still zero PR-blocking: the gate verdict is written (nightly) or logged (PR) but never changes any run's pass/fail. - history_gate.py: evaluate_gate gains `registry: CIRegistry | None`; uses the harness-selected registry (handles dual register_cuda_ci+register_rocm_ci files); no-spec files are vacuously trusted (never raise). - metric_history/neon_store.py: NeonMetricHistoryStore on psycopg[binary] (lazy import; transactional write_run across runs+metric_values; parameterized recent_trusted_values / mark_untrusted; no runtime DDL). psycopg[binary] added to requirements.txt. Live-Postgres smoke test skips without MILES_TEST_POSTGRES_DSN. - ci_utils.py / run_suite.py: is_nightly (GITHUB_EVENT_NAME=='schedule' or nightly label) + provenance helpers + build_store_from_env + run_gate_hook. After a CUDA PASS, evaluate the gate on the PASSING attempt's merged record; nightly writes a row (trusted from verdict + provenance), PR logs a shadow verdict (no write). The hook is wrapped so any gate/DB error is swallowed and never fails CI. - _run-ci.yml: expose NEON_DATABASE_URL (secrets inherit); pr-test.yml untouched. - docs/ci/03-metric-history-gate.md: scope writes to nightly-only; enforcement is later + allowlisted + kill-switchable. 207 tests pass, 1 skipped (live-PG smoke). Refs M2b/M3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| values=values, | ||
| ) | ||
| logger.info( | ||
| f"[CI Gate][nightly] {filename}: wrote baseline " f"(trusted={result.trusted}, {len(values)} value(s))" |
There was a problem hiding this comment.
Code Review
This pull request implements a CI metric history and regression gate system, including local NDJSON metric collection, AST-parsed gate specifications, offline and Neon Postgres storage backends with migrations, and integration into the CI test runner harness. The code review feedback focuses on enhancing robustness and resource management, recommending that database connections are reliably closed using try...finally blocks, abstracting the close method in the store interface, and adding defensive exception handling when merging records and initializing the database store to prevent CI runner crashes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # Regression-gate wiring: the store exists only when NEON_DATABASE_URL is | ||
| # set (CI), so the gate hook is a no-op locally. `is_nightly` is the | ||
| # baseline-writing signal (schedule event or stripped `nightly` label), not | ||
| # `args.nightly`. Provenance comes from the GitHub env. | ||
| gate_store = build_store_from_env() | ||
| gate_nightly = is_nightly(stripped_labels) | ||
| gate_provenance = gate_provenance_from_env() | ||
|
|
||
| return run_unittest_files( | ||
| ci_tests, | ||
| timeout_per_file=timeout, | ||
| continue_on_error=args.continue_on_error, | ||
| enable_retry=args.enable_retry, | ||
| max_attempts=args.max_attempts, | ||
| retry_wait_seconds=args.retry_wait_seconds, | ||
| gate_store=gate_store, | ||
| gate_nightly=gate_nightly, | ||
| gate_provenance=gate_provenance, | ||
| ) |
There was a problem hiding this comment.
To prevent resource leaks (such as unclosed database connections), it is highly recommended to wrap the execution of run_unittest_files in a try...finally block to ensure that gate_store.close() is always called if gate_store is successfully initialized.
# Regression-gate wiring: the store exists only when NEON_DATABASE_URL is
# set (CI), so the gate hook is a no-op locally. is_nightly is the
# baseline-writing signal (schedule event or stripped nightly label), not
# args.nightly. Provenance comes from the GitHub env.
gate_store = build_store_from_env()
gate_nightly = is_nightly(stripped_labels)
gate_provenance = gate_provenance_from_env()
try:
return run_unittest_files(
ci_tests,
timeout_per_file=timeout,
continue_on_error=args.continue_on_error,
enable_retry=args.enable_retry,
max_attempts=args.max_attempts,
retry_wait_seconds=args.retry_wait_seconds,
gate_store=gate_store,
gate_nightly=gate_nightly,
gate_provenance=gate_provenance,
)
finally:
if gate_store is not None:
gate_store.close()References
- To prevent resource leaks (e.g., counters that are not decremented), use constructs like try...finally or a with statement to ensure cleanup logic is always executed, even in the case of exceptions or early returns.
| def _merge_attempt_records(attempt_dir: str, merged_path: str) -> None: | ||
| """Merge every per-process NDJSON file under ``attempt_dir`` into one record. | ||
|
|
||
| Each per-process file holds lines of ``{"metric": key, "series": [[step, value], ...]}``. | ||
| The same metric key may appear across processes (driver + actors); concatenate | ||
| their series and sort by step so the merged per-run record is coherent. | ||
| """ | ||
| if not os.path.isdir(attempt_dir): | ||
| return | ||
| merged: dict[str, list[list]] = {} | ||
| for fname in sorted(os.listdir(attempt_dir)): | ||
| if not fname.endswith(".ndjson"): | ||
| continue | ||
| with open(os.path.join(attempt_dir, fname), encoding="utf-8") as f: | ||
| for line in f: | ||
| line = line.strip() | ||
| if not line: | ||
| continue | ||
| rec = json.loads(line) | ||
| merged.setdefault(rec["metric"], []).extend(rec["series"]) | ||
|
|
||
| for points in merged.values(): | ||
| points.sort(key=lambda p: (p[0] is None, p[0])) | ||
|
|
||
| with open(merged_path, "w", encoding="utf-8") as f: | ||
| for metric, points in merged.items(): | ||
| f.write(json.dumps({"metric": metric, "series": points}) + "\n") |
There was a problem hiding this comment.
To ensure robustness and defensive programming, _merge_attempt_records should catch potential exceptions (such as json.JSONDecodeError from corrupted/partially written files, or file access/permission errors) so that a malformed process record does not crash the entire CI runner process.
def _merge_attempt_records(attempt_dir: str, merged_path: str) -> None:
"""Merge every per-process NDJSON file under 'attempt_dir' into one record.
Each per-process file holds lines of '{"metric": key, "series": [[step, value], ...]}'.
The same metric key may appear across processes (driver + actors); concatenate
their series and sort by step so the merged per-run record is coherent.
"""
if not os.path.isdir(attempt_dir):
return
try:
merged: dict[str, list[list]] = {}
for fname in sorted(os.listdir(attempt_dir)):
if not fname.endswith(".ndjson"):
continue
with open(os.path.join(attempt_dir, fname), encoding="utf-8") as f:
for line in f:
line = line.strip()
if not line:
continue
try:
rec = json.loads(line)
merged.setdefault(rec["metric"], []).extend(rec["series"])
except (json.JSONDecodeError, KeyError, TypeError) as e:
logger.warning(f"Malformed line in {fname}: {e}")
continue
for points in merged.values():
points.sort(key=lambda p: (p[0] is None, p[0]))
with open(merged_path, "w", encoding="utf-8") as f:
for metric, points in merged.items():
f.write(json.dumps({"metric": metric, "series": points}) + "\n")
except Exception as e:
logger.warning(f"Failed to merge attempt records in {attempt_dir}: {e}")| def build_store_from_env(): | ||
| """Return the metric-history store for this environment, or None. | ||
|
|
||
| A hosted store is used only when ``NEON_DATABASE_URL`` is set (CI with the | ||
| secret inherited). Locally / in dev the var is unset and this returns None, | ||
| which disables the gate hook entirely -- no store, no evaluation, no writes. | ||
| """ | ||
| if os.environ.get(NEON_DATABASE_URL_ENV): | ||
| return NeonMetricHistoryStore() | ||
| return None |
There was a problem hiding this comment.
To prevent transient database connection failures or misconfigured environment variables from crashing the entire CI run on startup, build_store_from_env should catch any exceptions raised during the instantiation of NeonMetricHistoryStore and return None with a warning.
def build_store_from_env():
"""Return the metric-history store for this environment, or None.
A hosted store is used only when 'NEON_DATABASE_URL' is set (CI with the
secret inherited). Locally / in dev the var is unset and this returns None,
which disables the gate hook entirely -- no store, no evaluation, no writes.
"""
if os.environ.get(NEON_DATABASE_URL_ENV):
try:
return NeonMetricHistoryStore()
except Exception as e:
logger.warning(f"[CI Gate] Failed to initialize NeonMetricHistoryStore: {e}")
return None
return None| @abc.abstractmethod | ||
| def mark_untrusted( | ||
| self, | ||
| *, | ||
| run_id: str | None = None, | ||
| github_run_id: int | None = None, | ||
| commit_sha: str | None = None, | ||
| ) -> int: | ||
| """Revoke trust on the runs matching exactly one of the given keys. | ||
|
|
||
| Returns the number of run rows whose ``trusted`` flag changed from true | ||
| to false. Already-untrusted matches do not count. Revocation takes | ||
| effect immediately for subsequent ``recent_trusted_values`` calls; no | ||
| rebaseline step is required. | ||
|
|
||
| Exactly one of ``run_id`` / ``github_run_id`` / ``commit_sha`` must be | ||
| provided. | ||
| """ |
There was a problem hiding this comment.
It is good practice to define a close method in the abstract base class MetricHistoryStore so that callers can uniformly close any store implementation to release underlying resources.
@abc.abstractmethod
def mark_untrusted(
self,
*,
run_id: str | None = None,
github_run_id: int | None = None,
commit_sha: str | None = None,
) -> int:
"""Revoke trust on the runs matching exactly one of the given keys.
Returns the number of run rows whose 'trusted' flag changed from true
to false. Already-untrusted matches do not count. Revocation takes
effect immediately for subsequent 'recent_trusted_values' calls; no
rebaseline step is required.
Exactly one of 'run_id' / 'github_run_id' / 'commit_sha' must be
provided.
"""
def close(self) -> None:
"""Close the store and release any underlying resources."""
pass|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
No description provided.