-
Notifications
You must be signed in to change notification settings - Fork 1
fix: [no-ticket] harbor - honest measurement signals (summary qualifiers, versioned re-evals, dead-attempt causes) #35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: harbor-7-access-tiers
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,6 +82,7 @@ def __init__( | |
| # but at >= k times the sample budget per label. <= 1 disables the floor. | ||
| self.k_anonymity_floor = k_anonymity_floor | ||
| self._free_baseline_used = False | ||
| self._eval_seq = 0 # per-eval ordinal for result-dir versioning | ||
|
|
||
| # ------------------------------------------------------------------ | ||
| # Handlers (the HTTP layer resolves `admin` from auth and calls these) | ||
|
|
@@ -275,24 +276,52 @@ def _route_results(self, experiment: Experiment, *, admin: bool) -> str | None: | |
| return None | ||
|
|
||
| commit = experiment.run.candidate.commit | ||
| dest = self.agent_volume / "results" / f"{split}__{commit[:12]}" | ||
| # Recreate the dir so it reflects exactly this metered run. The dir is keyed | ||
| # only on (split, commit[:12]); a prior eval of the same commit on a larger | ||
| # sample set would otherwise leave stale per-sample files behind that this | ||
| # run did not produce, and result_path would surface them as if they were. | ||
| if dest.exists(): | ||
| # Every metered eval gets its own versioned dir. Keying on | ||
| # (split, commit) alone forced a wipe-and-rewrite, so a re-measurement | ||
| # (a multifidelity confirm, a noise re-eval of the champion) erased the | ||
| # agent's earlier evidence for the same commit; repeat measurements are | ||
| # exactly the ones worth comparing. result_path in the response names | ||
| # the dir for THIS eval. | ||
| self._eval_seq += 1 | ||
| dest = ( | ||
| self.agent_volume | ||
| / "results" | ||
| / f"{split}__{commit[:12]}__e{self._eval_seq}" | ||
| ) | ||
| if dest.exists(): # ordinal collision only on volume reuse; never merge | ||
| shutil.rmtree(dest) | ||
| dest.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| # Aggregate summary is label-safe for both visible and partial tiers. | ||
| # n_scored / n_errored / score_se qualify the mean: a mean over 3 | ||
| # scored samples of 18, or one dominated by errored zero-fills, is a | ||
| # different measurement than a clean full-split mean, and the agent | ||
| # (and any auditor) should see that without per-sample access. | ||
| sample_results = experiment.result.sample_results | ||
| filled = [ | ||
| r.score if r.score is not None else 0.0 | ||
| for r in sample_results.values() | ||
| ] | ||
| score_se = None | ||
| if len(filled) > 1: | ||
| m = sum(filled) / len(filled) | ||
| var = sum((x - m) ** 2 for x in filled) / (len(filled) - 1) | ||
| score_se = (var / len(filled)) ** 0.5 | ||
|
Comment on lines
+301
to
+309
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: vero/src/vero/harbor/server.py
Line: 301-309
Comment:
**`score_se` is the SE of the zero-filled mean, but `n_scored` describes only the non-error population**
`filled` zero-fills every sample whose `score is None` before computing the variance, which is consistent with `mean_score` (both use `default_minimum_score = 0.0` as the fill). However, an agent reading `n_scored=3` alongside `score_se` will naturally interpret SE as the uncertainty of the 3-sample mean — but it is actually the SE of the zero-filled-18-sample mean, which is smaller by roughly `sqrt(n_scored / n_samples)`. Concretely: 3 of 18 scored with [1, 0.8, 0.9] gives SE ≈ 0.083 (zero-filled) vs ≈ 0.058 (scored-only); a reader inferring noise from the zero-filled SE will underestimate it. Adding a `score_se_n` field or a comment in the JSON clarifying that `score_se` is computed over `n_samples` (not `n_scored`) would avoid the misread.
How can I resolve this? If you propose a fix, please make it concise.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair: the pairing of n_scored with an SE computed over the zero-filled population invited misreading. Fixed in e1b1d6b (stack tip): renamed to |
||
| (dest / "summary.json").write_text( | ||
| json.dumps( | ||
| { | ||
| "split": split, | ||
| "commit": commit, | ||
| "n_samples": len(experiment.result.sample_results), | ||
| "n_samples": len(sample_results), | ||
| "n_scored": sum( | ||
| 1 for r in sample_results.values() if r.score is not None | ||
| ), | ||
| "n_errored": sum( | ||
| 1 for r in sample_results.values() if r.is_error() | ||
| ), | ||
| "mean_score": experiment.result.score(), | ||
| "status": str(experiment.result.status), | ||
| "score_se": score_se, | ||
| "status": experiment.result.status.value, | ||
| }, | ||
| indent=2, | ||
| ) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says "ordinal collision only on volume reuse; never merge", but when a server restarts with a reused volume the first N evals quietly wipe whatever
__e1through__eNdirs survived from the prior session. Since the entire motivation of the versioned-dir change is that a re-measurement must not erase earlier evidence, a silentshutil.rmtreehere is exactly the failure mode the PR is trying to prevent — it just occurs on restart rather than on same-session re-eval. At minimum alogger.warning(...)at this branch would surface the collision to auditors; the alternative is a guard on the server startup that moves the conflicting dirs to a timestamped backup name.Prompt To Fix With AI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, restart-with-reused-volume was exactly the erasure this change is supposed to prevent. Fixed in e1b1d6b (stack tip): on the first routed eval,
_route_resultsscans the results root for surviving__eNdirs and resumes the ordinal past the max, so a restarted sidecar appends (e8, e9, ...) instead of wiping e1. Test:test_volume_reuse_resumes_ordinal_past_survivors.