fix: hybrid-merge cached mirror PRs when only mirror fetch fails - #830
Conversation
anderdc
left a comment
There was a problem hiding this comment.
The bug is real and the diagnosis is correct, but the hybrid-merge approach is more invasive than the situation warrants given the legacy path is on the chopping block.
The whole combine module is documented as going away on delete-day ("On delete-day this whole module goes away — at that point MirrorMinerEvaluation becomes the canonical container"). Once legacy is gone:
mirror_pr_fetch_failedcollapses back into a single failure flag- The mixed-source problem stops existing entirely
- A mirror outage just means "no data this round, use cached eval" — same shape as legacy works today
Everything the hybrid splice adds (_splice_cached_mirror_into, the dedicated conditional block, the re-cache step) is transition-only scaffolding that we'll then have to delete. And during the transition it produces a mixed-snapshot eval (fresh legacy PRs + stale-cached mirror PRs evaluated together) where credibility and other cross-PR multipliers get recomputed across a Frankenstein view of the miner's state.
Suggested simpler fix:
-
Keep the
mirror_pr_fetch_failedflag and thecombinewiring — that part is necessary signal. -
In
store_or_use_cached_evaluation, just let mirror failures through the existing cache-fallback path instead of building a new branch. Roughly:if not miner_eval.should_use_cache_fallback and not miner_eval.mirror_pr_fetch_failed: bt.logging.warning(...) continue cached_eval = self.evaluation_cache.get(...) if cached_eval is not None: bt.logging.info(...) miner_evaluations[uid] = cached_eval cached_uids.add(uid) -
Drop
_splice_cached_mirror_intoand the dedicated hybrid block inneurons/validator.pyentirely.
The trade-off: during a mirror outage, mixed-source miners get evaluated against a coherent one-round-stale snapshot instead of a fresh-legacy + stale-mirror hybrid. They lose the freshness of this round's legacy PRs for one cycle. Given mirror outages are transient and we're heading toward mirror-only anyway, that's the right trade — less code, no cross-source contamination, and nothing extra to delete on delete-day.
Tests: test_combine.py assertions stay; the validator-side tests can be simplified to assert "mirror failure with cached eval available → swapped to cached eval, returned in cached_uids."
When a mirror fetch fails but the legacy PR list loads cleanly, combine.combine previously dropped the mirror contributions to zero before they reached the validator. The miner was then evaluated against a fresh-legacy + zeroed-mirror view for that round, silently losing mirror-hosted work and letting cross-PR multipliers (credibility, issue scoring) recompute over an incomplete state. Materialize mirror_pr_fetch_failed on MinerEvaluation alongside the existing OR into github_pr_fetch_failed, and extend store_or_use_cached_evaluation's early-skip check so that a mirror outage routes the miner through the existing cache-fallback path the same way a legacy outage does. The miner is then evaluated against a coherent one-round-stale snapshot rather than a mixed-source hybrid. Tests cover the new flag on combine and the swap-to-cached behavior in the validator.
9dfde3d to
873257a
Compare
…ache-fallback-mixed-source # Conflicts: # tests/validator/test_validator_cache_fallback.py
|
pushed the update |
Summary
Mixed legacy + mirror miners are silently zeroed when
mirror.gittensor.iois down.combineOR'smirror_eval.fetch_failedinto the legacygithub_pr_fetch_failed, butshould_use_cache_fallbackstill requirestotal_prs == 0— so a successful legacy fetch (1+ PR) blocks the fallback and the validator finalizes a partial evaluation missing every mirror PR. Reach: any miner with PRs in both anentrius/*mirror repo and any non-mirror repo, during a mirror outage. Drops pastMIN_VALID_MERGED_PRS→ 0 OSS reward.Adds a per-source
mirror_pr_fetch_failedflag (mirror is binary — atomic load), wires it throughcombine, and routes mixed mirror-only failures through a hybrid merge instore_or_use_cached_evaluation: keep the fresh legacy data, splice cached mirror PRs back in, and re-cache the result. CachedScoredMirrorPRreuse is safe becausebase_score/token_scoreare deterministic over the merged diff; credibility and other cross-PR multipliers get recomputed infinalize_miner_scores.The pre-mirror legacy partial-pagination semantic (#516) is preserved — that path uses the existing
should_use_cache_fallbackgate untouched. Pure-mirror miners and full-failure cases also keep the existing full-eval fallback.Related Issues
Closes #681
Type of Change
Testing
Checklist