PR 05: Submit and materialize samples#124
Conversation
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
| remain provenance rows; they are not serialized into the runtime replay | ||
| contract. `workflow/started` may now carry only `sample_id` when the sample | ||
| graph already exists, while the definition-backed initialization path remains as | ||
| a temporary bridge until the runtime consolidation PR removes it. |
There was a problem hiding this comment.
everywhere where this has been left as a temporary bridge we should mark with a TODO to delete in a given PR (ie TODO: PR 9 removes this when we do x")
| definition = require_not_none( | ||
| session.get(ExperimentDefinition, command.definition_id), | ||
| f"Definition {command.definition_id} not found", | ||
| ) |
There was a problem hiding this comment.
similar to downstack PR: double check if this breaking wider domain patterns by mixing the "data layer" and the "service layer" on the same object
| ) | ||
|
|
||
|
|
||
| def _resolve_sample_worker_config( |
There was a problem hiding this comment.
this whole added block seems pretty messy. find some way to impliment this more cleanly (maybe lean on pydantic to parse more?) or reply to this comment justifying why not
| from ergon_core.core.persistence.telemetry.models import SampleRecord | ||
|
|
||
|
|
||
| def component_slug(snapshot: Mapping[str, JsonValue], *, fallback: str) -> str: |
There was a problem hiding this comment.
again, this fallbacking is super messy. we shouldn't be needing to write all of this parsing code in my mind if we have a strong type system
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
35dc75c to
5e5adf6
Compare
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
9a884eb to
815a331
Compare
5e5adf6 to
261f3bb
Compare
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
1 similar comment
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
261f3bb to
faa0aae
Compare
815a331 to
98fe37b
Compare
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
1 similar comment
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
|
Closed after landing the cumulative episode/sample stack snapshot through PR132 directly onto dev in merge commit 1c22130. This PR's changes are included in dev. |
1 participant
Review Guide
Stack position: PR 05, base
codex/episode-sample-pr04-experiment-persistence, headcodex/episode-sample-pr05-submit-materialize.Implements RFC Docs
02-core-authoring-api.md:Experiment.submit, candidate fill, sampler selection, selected sample persistence.03a-persistence-schemas.md: runtime sample identity, sampler invocations, typed WAL.03b-implementation-churn.md: definition-id bridge and sample materialization replay boundary.09-implementation-plan/06-pr-05-submit-materialize-samples.md.Why This PR Exists
This is where authored experiments start becoming runnable runtime state. It connects persisted candidate pools, samplers, sample materialization, typed WAL initialization, and workflow start events.
What Actually Changes
core/application/experiments/submission.py.SampleCandidatePool, calls the sampler withSamplingContext, recordsExperimentSamplerInvocationRow, marks selected entries, creates runtimeSampleRecords, commits, and emitsWorkflowStartedEvent(sample_id=...).core/application/samples/materialization.py.SampleGraphNode,SampleGraphEdge, and typed WAL rows from selected publicSample.tasks.definition_idand materialized graph rows.Gotchas / Review Risks
WorkflowStartedEvent.model_dump()omitsNone, so sample-only events do not includedefinition_id. Generated frontend contracts must stay in sync.environment_name + sample_key; duplicate keys inside an environment are ambiguous.SampleRecordstill carries some legacy compatibility fields, such asbenchmark_type="experiment",instance_key, and sample-key-derived IDs.EmptyTaskPayload.Cleaned Later
Validation Notes
Key tests include
test_experiment_submit.py,test_sample_materialization.py, andtest_submit_starts_materialized_sample.py. At the time of review, inherited CI failures included stale generated workflow schemas, a missing old DTO import, and smoke migration order issues.