Repository navigation
Conversation
Birfy
left a comment
There was a problem hiding this comment.
The multi-artifact path is otherwise covered by green CI, but the contract-breaking commit does not fulfill the atomic adaptation contract introduced here. _commit_with_retry() accepts adapters, yet its _decide() call site does not pass dependent artifact states; the transaction therefore commits only [candidate]. A breaking contract can land while Contract.depends_on artifacts remain unchanged. Please supply and validate the adapted dependents in the same commit_atomic() call, or narrow the API/docs and add an explicit rejection when required adapters are missing.
…— worker loop, PP stages, atomic contract changes evolve() and async_evolve() register any number of artifacts beside the primary. The worker loop proposes against each (PP stages name them; the stages must resolve to registered artifact ids), the aggregator merges their diffs by target (the evidence buffer already bucketed by diff.target), and a contract-breaking diff lands atomically through Ledger.commit_atomic — the one path the ledger sanctions for a deliberate contract change, previously unreachable from any engine path. Contract.depends_on declares the reverse dependency graph: when a registered artifact commits a contract-breaking change, dependents' cached evaluations are evicted so they re-measure under the new contract (MemoryCache/FileCache gain invalidate(rendered)). EvolveSpec carries extra_artifacts; docs/api.md regenerated; 10 tests in tests/test_multi_artifact.py.
…se a breaking change without adapted dependents _commit_with_retry accepted `adapters` but _decide never supplied dependent states, so a contract-breaking commit silently landed as just the candidate — leaving artifacts whose `Contract.depends_on` names it registered against a superseded contract (PR Birfy#197 review). - _decide refuses a contract-breaking diff on an artifact with declared dependents when no adapted states are provided (new `missing-adapters` MergeOutcome), rather than half-applying the break. - When adapters are supplied, the atomic commit now CAS-checks every state it writes: the base version vector names the candidate and each adapter, so the whole transaction is genuinely all-or-nothing. Tests: refusing a breaking change with dependents but no adapters; committing candidate + adapters in one atomic commit and re-measuring dependents; direct Engine.invalidate_dependents eviction.
7ff7ea0 to
ec9bd9c
Compare
|
Addressed the review: the atomic adaptation transaction is now enforced, not assumed. The problem: The fix (two parts):
Tests:
Also rebased onto current upstream |
Birfy
left a comment
There was a problem hiding this comment.
Re-reviewed ec9bd9c. Missing-all adapters now correctly rejects, the complete happy path commits all supplied states, and the initial CAS vector covers them. 91 focused shipped tests passed. Two supplied-adapter safety gaps remain; independently reproduced using the same private _Candidate/ledger setup as the new atomic-adapter tests:
-
_decideonly tests whether the adapter list is nonempty (aggregator.py:1364–1366), not whether it covers the required dependent IDs. With declared dependents [b,c] and only adapter b, the breaking change commits artifact v2 and b v2 while c stays v1. With required dependent b and an unrelated adapter instead, it also commits while b remains v1. Please validate complete required-ID coverage, plus adapter identity/duplicate handling, before committing. Add partial-set and wrong-target regression cases, not just empty versus complete lists. -
CAS retry can overwrite a concurrent adapter update. Prepare the primary candidate and b adapter from snapshot b v1, then concurrently commit b v2 containing
concurrent='keep me'. The first atomic CAS correctly fails. The retry (aggregator.py:1167–1171) refreshes the head and rebases only the primary candidate, retaining the old adapter state; it then expects b v2 and writes that stale adapter as b v3, deleting the concurrent field. Please reject the stale adaptation transaction or rebuild/rebase and revalidate every affected adapter; do not advance adapter expected versions without updating their states. Add a conflict test that verifies the competing adapter update survives, or that the whole transaction is safely refused.
The extra probes produced three failures (partial set, wrong target, stale-adapter overwrite) and two passing controls (missing-all rejection, complete atomic commit). The ordinary worker preparation path still does not populate adapters, so its safe rejection is a useful improvement; the supplied-adapter path being claimed/tested here still needs these guards before it is safe for custom integration.
Separate integration status: exact-head tests run 38023236790 fails across Python 3.9/3.10/3.11/3.12. Each inspected failure summary lists the shared install-page anchor and port-count tests; docs build 38023236789 succeeds. Those shared documentation failures should be corrected and the resulting exact-head matrix rerun, independently of the adapter findings.
What this does
evolve()andasync_evolve()can now evolve any number of artifacts, not just the primary. This is the first half of the design's L-task cross-product (task-cluster × artifact): the second axis finally has somewhere to live.What changed
extra_artifacts=— register more artifactsBoth engines take
extra_artifacts: {aid: EvolvingArtifact}besideartifact_id. Each extra carries its ownblast_radius,strategyandContract. The worker loop rolls each out and proposes against it, so a proposal can name any registered artifact as its diff target.Worker loop proposes against every artifact
_artifact_targets/_propose_oneinevolution.py): DP+extras rolls out primary and each extra; PP proposes against the stage's artifact only.async_evolve.py): same cross-product on the barrier-free path (no parallel strategy, so it is primary + every extra).PipelineParallelis no longer refused_reject_pipeline_parallellifts onceextra_artifactsare registered, and each PP stage must name a registered artifact (unknown stages raise, not silently drop).evolve(parallel=PipelineParallel(...))without extras still raises as before.Contract dependency graph + atomic adaptation
Contractgainsdepends_on(artifact ids whose contracts it relies on).Ledger.commit_atomic— the only path the ledger sanctions for a deliberate contract change, previously unreachable from any engine path (commit_atomicwas "provided and tested but not used").MemoryCache/FileCache.invalidate(rendered)) so they re-measure under the superseded-then-replaced contract.EvolveSpecsupportsextra_artifactscompose()merges the field intoextra_artifacts=forevolve().Test & docs
tests/test_multi_artifact.py— 10 tests: registration, cross-artifact diffs, per-artifact strategies, PP allowed/refused/unknown-stage, atomic contract commit, dependent cache eviction, commit_atomic all-or-nothing.test_era_algotune/test_porous_moleculesfail onmaintoo).docs/api.mdregenerated (tools.gen_api_docs);concepts,aggregator,data-model,modulesupdated;LedgerProtocolgainscommit_atomic; CHANGELOG updated.Follow-up (Phase 2)
TaskClusterartifact dimension — the triggering side of L-task (which artifact a task's failure implicates).adapters=in_commit_with_retry— worker-produced cross-artifact adapter states for the full atomic adaptation transaction.