PR 03: Add public sample authoring API - #122
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 —
|
c323335 to
c20f49c
Compare
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
E2E smoke —
|
|
|
||
|
|
||
| @runtime_checkable | ||
| class ExperimentSubmissionService(Protocol): |
There was a problem hiding this comment.
this should be moved to core marked explicitly with a TODO for the PR in which it is intended to land
| raise NotImplementedError("Streaming environments may not materialize all samples") | ||
| return list(self.iter_samples()) | ||
|
|
||
| def iter_samples(self) -> Iterator[Sample]: |
There was a problem hiding this comment.
should be marked with a todo for where we plan on implimenting in what PR
|
|
||
|
|
||
| @runtime_checkable | ||
| class ExperimentPersistenceService(Protocol): |
There was a problem hiding this comment.
should be moved to core with a todo on when we plan to impliment
| from ergon_core.api.benchmark import Task | ||
|
|
||
|
|
||
| def _task_key(task: Task) -> str: |
There was a problem hiding this comment.
inline this logic in its call sites
| return task.task_slug | ||
|
|
||
|
|
||
| def _task_dependencies(task: Task) -> tuple[str, ...]: |
There was a problem hiding this comment.
inline this logic in its call sites
|
|
||
|
|
||
| @runtime_checkable | ||
| class ExperimentSubmissionPort(Protocol): |
There was a problem hiding this comment.
I can't see anywhere on PR#2 where this is actually implimented, double check for me why we need this? should it be flattened
| ) -> "ExperimentSubmitResult": ... | ||
|
|
||
|
|
||
| class ExperimentRef(BaseModel): |
There was a problem hiding this comment.
come up with a better name for this schema
|
|
||
|
|
||
| @runtime_checkable | ||
| class SamplingHistory(Protocol): |
There was a problem hiding this comment.
this shouldn't really be a protocol (I can't see it constructed anywhere feels like itt could be a method on SamplingContext
|
|
||
|
|
||
| @runtime_checkable | ||
| class Sampler(Protocol): |
There was a problem hiding this comment.
similarly: I think this should be a BaseModel with an Abstractmethod that users have to subclass to make it clearer this is a concrete base class (like we do for experiment)
|
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 03, base
codex/episode-sample-pr02-rollout-batch-samples, headcodex/episode-sample-pr03-public-authoring-api.Implements RFC Docs
01-exec-summary.md: introducesExperiment,Environment,Sample, andSampleras the new authoring vocabulary.02-core-authoring-api.md: public package surface and submit ergonomics.03-core-runtime-persistence-fe.md: locked decisions arounditer_samples, optionalall_samples, and sampler boundaries.09-implementation-plan/04-pr-03-public-sample-authoring-api.md.Why This PR Exists
This PR creates the public authoring seam without making it responsible for persistence or runtime execution. Users can describe samples, environments, experiments, and samplers, while later PRs wire those objects into persistence and materialization.
What Actually Changes
ergon_core.api.experimentwith:SampleEnvironmentExperimentExperimentRefExperimentSubmitResultSamplerRandomSamplerSamplingContextSamplingHistorypersist_experimentSamplevalidates real publicTaskvalues, task slugs, dependencies, root tasks, and lookup by task key.Experiment.submit(...)validates and delegates to an injected submission service.persist_experiment(...)delegates through a thin public port.ergon_core.apiexports the beginner-facing names.Gotchas / Review Risks
RandomSampler.select(...)returns every candidate in randomized order; it does not truncate tok. In this design, upstream candidate request sizing controlsk.Environment.iter_candidate_samples()and the stream cursor land here before candidate-pool persistence, so they look ahead to PR 04.Episode,SourceDescriptor,EnvironmentSource, andExperimentRunHandleout of the public surface.test_public_experiment_submit_smoke.pyimports the futureExperimentSubmissionServiceand is strict-xfailed until PR 05.Cleaned Later
Environment.iter_samples()implementations.persist_benchmarkand definition launch remain until the deletion PR.Validation Notes
The main review target is API shape and boundary tests, not runtime behavior yet. Current CI on this layer had lower-stack issues at the time of review: a stale frontend workflow-started schema and a
RunCommunicationMessageDtoimport failure from inherited code.