diff --git a/.gitignore b/.gitignore index 3bcd6c5f..f5b35deb 100644 --- a/.gitignore +++ b/.gitignore @@ -13,6 +13,7 @@ Docs/plan_suggestions/ Docs/commit_reviews/ Docs/plan_reviews/ +Docs/ab_testing_generalization_plan.md # Python __pycache__/ diff --git a/Docs/ab_testing_generalization_plan.md b/Docs/ab_testing_generalization_plan.md deleted file mode 100644 index cd96e379..00000000 --- a/Docs/ab_testing_generalization_plan.md +++ /dev/null @@ -1,372 +0,0 @@ -# Generalize Pipeline to Control/Treatment A/B Framework - -## Motivation - -The pipeline currently hardcodes "skilled vs unskilled" as the only experiment type. This refactor generalizes it to support arbitrary A/B experiments (skills, prompts, models, tools) while keeping skills as the default. - -**Terminology:** "control" = baseline (no treatment applied), "treatment" = the intervention being tested. For the current skill experiment: treatment = with skills/docs, control = without. - -## Design - -```mermaid -flowchart TD - subgraph metadata ["metadata.yaml"] - ExperimentConfig["experiment:\n type: skill\n n_trials: 20\n treatment/control spec"] - end - - subgraph scaffold ["Scaffolder"] - Strategy["ExperimentStrategy\n(Protocol)"] - SkillStrategy["SkillStrategy\n(default)"] - ModelStrategy["ModelStrategy"] - ConfigDriven["ConfigDrivenStrategy\n(custom types)"] - Strategy --> SkillStrategy - Strategy --> ModelStrategy - Strategy --> ConfigDriven - end - - subgraph output ["Output"] - ControlDir["tasks-control/name/"] - TreatmentDir["tasks-treatment/name/"] - end - - metadata --> scaffold - scaffold --> output -``` - -## Schema Changes (`abevalflow/schemas.py`) - -Add experiment configuration to `SubmissionMetadata`: - -```python -class ExperimentType(StrEnum): - SKILL = "skill" - MODEL = "model" - PROMPT = "prompt" - CUSTOM = "custom" - -class CopySpec(BaseModel): - """A source directory and its destination path inside the container.""" - src: str = Field(description="Directory name in submission (e.g. 'skills')") - dest: str = Field(description="Absolute path in container (e.g. '/skills')") - - @field_validator("src") - @classmethod - def _strip_trailing_slash(cls, v: str) -> str: - return v.rstrip("/") - - @field_validator("src") - @classmethod - def _reject_path_traversal(cls, v: str) -> str: - if ".." in v or v.startswith("/"): - raise ValueError("src must be a relative top-level directory name") - return v - -class VariantSpec(BaseModel): - copy: list[CopySpec] = Field(default_factory=list) - env_from_secrets: dict[str, str] = Field( - default_factory=dict, - description=( - "Env vars to inject at runtime via OpenShift Secrets. " - "Keys are env var names, values are secret references " - "(e.g. 'secret-name/key'). Raw values are NOT allowed." - ), - ) - - @model_validator(mode="after") - def _no_duplicate_src(self) -> "VariantSpec": - srcs = [c.src for c in self.copy] - if len(srcs) != len(set(srcs)): - raise ValueError("Duplicate src directories in copy spec") - return self - -class ExperimentConfig(BaseModel): - type: ExperimentType = Field( - default=ExperimentType.SKILL, - description="Experiment type: skill, model, prompt, custom", - ) - n_trials: int = Field(default=20, gt=0, le=100, description="Number of trials per variant") - treatment: VariantSpec = Field( - default_factory=lambda: VariantSpec( - copy=[CopySpec(src="skills", dest="/skills"), CopySpec(src="docs", dest="/workspace/docs")] - ), - ) - control: VariantSpec = Field(default_factory=VariantSpec) -``` - -Key design decisions: - -- **`CopySpec(src, dest)` tuples** instead of plain dir names — `skills/` must copy to `/skills/` (Harbor contract), while `docs/` goes to `/workspace/docs/`. The Dockerfile template uses these pairs directly. -- **`env_from_secrets`** instead of raw `env: dict[str, str]` — prevents secret leakage in metadata.yaml. Values reference OpenShift Secrets (`secret-name/key`), resolved at runtime via `persistent_env` in Harbor, not baked into the Dockerfile. -- **`ExperimentType` is a `StrEnum`** — unknown types raise `ValidationError` at schema validation, not silently falling through. -- **`n_trials`** has an upper bound (`le=100`) to prevent accidental resource exhaustion. -- Bare directory names (no trailing slashes) enforced by `field_validator`. -- Path traversal (`../`, absolute paths) rejected in `src`. -- Duplicate `src` directories rejected by `model_validator`. - -## Strategy Pattern (`abevalflow/experiment.py` — new file) - -```python -class ExperimentStrategy(Protocol): - def variant_copy_specs( - self, submission_dir: Path, variant: str, - ) -> list[CopySpec]: - """Return copy specs for this variant (control or treatment).""" - - def customize_context(self, base_context: dict, variant: str) -> dict: - """Adjust template context per variant. - - Must set 'skills_dir' to '/skills' when skills/ is in the copy - spec, and omit/None it otherwise. This drives the task.toml.j2 - conditional for skills_dir. - """ - -class SkillExperimentStrategy: - """Default strategy: treatment includes skills/docs, control excludes them. - - customize_context sets: - - treatment: skills_dir='/skills', copy_pairs=[('skills','/skills'), ...] - - control: skills_dir=None, copy_pairs=[('supportive','/workspace/supportive'), ...] - """ - -class ModelExperimentStrategy: - """Same files for both variants, different env vars. - - Both variants get identical copy specs. The difference is in - env_from_secrets — e.g., treatment uses model A, control uses model B. - env vars are injected via Harbor's persistent_env at runtime, - NOT as Dockerfile ENV directives. - """ - -class ConfigDrivenStrategy: - """Reads copy/env directly from ExperimentConfig for 'custom' type. - - Sets skills_dir='/skills' when 'skills' is in the copy spec src list. - """ -``` - -Factory function: - -```python -_STRATEGY_MAP: dict[ExperimentType, type[ExperimentStrategy]] = { - ExperimentType.SKILL: SkillExperimentStrategy, - ExperimentType.MODEL: ModelExperimentStrategy, - ExperimentType.PROMPT: SkillExperimentStrategy, # same file logic, different content - ExperimentType.CUSTOM: ConfigDrivenStrategy, -} - -def get_strategy(config: ExperimentConfig) -> ExperimentStrategy: - cls = _STRATEGY_MAP[config.type] - return cls(config) -``` - -No silent fallback — `ExperimentType` enum + dict lookup guarantees a `KeyError` for unmapped types (which can't happen since the enum validates at schema level). - -### `skills_dir` contract - -The strategy's `customize_context` is responsible for setting `skills_dir` in the template context: - -- If `"skills"` is in the variant's copy spec `src` list → set `skills_dir = "/skills"` (or the matching `dest`) -- Otherwise → set `skills_dir = None` - -This drives `task.toml.j2`: -```jinja2 -{% if skills_dir %} -skills_dir = "{{ skills_dir }}" -{% endif %} -``` - -## Scaffold Refactor (`scripts/scaffold.py`) - -Key changes: -- Replace `SKILLED_COPY_DIRS` / `UNSKILLED_COPY_DIRS` constants with strategy-driven `CopySpec` lists -- Rename output dirs: `tasks//` → `tasks-treatment//`, `tasks-no-skills//` → `tasks-control//` -- `_build_template_context` populates `copy_pairs` (from `CopySpec`) instead of individual `has_*` booleans -- `scaffold_submission()` returns `(control_dir, treatment_dir)` instead of `(skilled_dir, unskilled_dir)` -- Accept `ExperimentConfig` (from parsed metadata) and delegate to strategy -- Common dirs (`tests/`, `supportive/`, `scripts/`) are always copied for both variants; the strategy only controls the treatment-specific dirs - -## Template Unification (`templates/`) - -### Merge Dockerfiles into one: `Dockerfile.j2` - -Replace `Dockerfile.skilled.j2` and `Dockerfile.unskilled.j2` with a single template: - -```jinja2 -FROM registry.access.redhat.com/ubi9/python-311:latest - -USER 0 -RUN dnf install -y --quiet curl \ - && curl -LsSf https://astral.sh/uv/0.9.7/install.sh | env UV_INSTALL_DIR=/usr/local/bin sh \ - && dnf clean all -USER 1001 - -WORKDIR /workspace - -COPY instruction.md . -COPY tests/ /tests/ -{% for src, dest in copy_pairs %} -COPY {{ src }}/ {{ dest }}/ -{% endfor %} -``` - -`copy_pairs` is a list of `(src, dest)` tuples built from `CopySpec`. Example for skill treatment: `[("skills", "/skills"), ("docs", "/workspace/docs"), ("supportive", "/workspace/supportive")]`. For control: `[("supportive", "/workspace/supportive")]`. - -When `copy_pairs` is empty (e.g., control with no optional dirs), no extra `COPY` lines are emitted — the Dockerfile is still valid. - -### `task.toml.j2` - -Replace `{% if variant == "skilled" %}` with `{% if skills_dir %}`: - -```jinja2 -{% if skills_dir %} -skills_dir = "{{ skills_dir }}" -{% endif %} -``` - -### `test.sh.j2` - -No changes needed — `has_llm_judge` is still determined by directory inspection in `_build_template_context`, independent of the experiment strategy. - -### Delete `Dockerfile.skilled.j2` and `Dockerfile.unskilled.j2` - -## Tekton YAML Renames - -### `pipeline/tasks/build-push.yaml` -- Params: `skilled-task-dir` / `unskilled-task-dir` → `treatment-task-dir` / `control-task-dir` -- Results: `skilled-image-ref` / `unskilled-image-ref` → `treatment-image-ref` / `control-image-ref` -- Step names: `build-push-skilled` / `build-push-unskilled` → `build-push-treatment` / `build-push-control` -- Image tags: `:skilled-` / `:unskilled-` → `:treatment-` / `:control-` - -### `pipeline/tasks/scaffold.yaml` -- Results: `skilled-task-dir` / `unskilled-task-dir` → `treatment-task-dir` / `control-task-dir` - -### Future `pipeline/tasks/harbor-eval.yaml` -- Will use `treatment-image-ref` / `control-image-ref` and `n-trials` param - -### `n_trials` flow (to be investigated) - -`n_trials` flows: `metadata.yaml` → scaffold reads it → emits as Tekton result → `harbor-eval.yaml` param → `harbor run` CLI flag. - -**Investigation needed:** Verify Harbor's CLI interface for trial count (`--runs`, `--n-trials`, or config-based). If Harbor doesn't support this via CLI, `n_trials` may need to be written into `task.toml` or passed as an environment variable. This investigation should happen before implementing the `harbor-eval.yaml` task. - -## Test Updates - -### `tests/test_scaffold.py` -- Rename all `skilled`/`unskilled` references to `treatment`/`control` -- Concrete test cases: - - **Backward compat:** Submission with no `experiment` key produces same output as today (`tasks-treatment/` with skills at `/skills/`, `tasks-control/` without) - - **Model strategy env:** `ExperimentConfig(type="model", ...)` with `env_from_secrets` populates context correctly for both variants - - **Custom strategy parity:** `ConfigDrivenStrategy` with `copy=[CopySpec(src="skills", dest="/skills")]` produces identical output to `SkillExperimentStrategy` - - **Copy path correctness:** Skills copied to `/skills/` (not `/workspace/skills/`), docs to `/workspace/docs/` - - **Empty copy_pairs:** Control with no optional dirs produces valid Dockerfile (no COPY lines after tests) - - **n_trials passthrough:** Verify `n_trials` value is accessible after scaffold - -### `tests/test_validate.py` -- `ExperimentConfig` validation: valid types, invalid type rejected, `n_trials` bounds -- `VariantSpec` validation: path traversal rejected, duplicate src rejected -- `CopySpec` validation: trailing slash stripped, absolute src rejected -- `env_from_secrets` format validation - -## Security Rules - -- **NEVER put raw API keys or secrets in `metadata.yaml`** — use `env_from_secrets` which references OpenShift Secret names, not literal values -- `env_from_secrets` values are resolved at runtime via Harbor's `persistent_env` mechanism, injected into trial Pods from cluster Secrets -- `validate.py` should reject `metadata.yaml` files containing common secret patterns (e.g., keys matching `sk-*`, `AKIA*`) - -## Backward Compatibility - -- If `metadata.yaml` has no `experiment` section, default to `ExperimentConfig()` which produces skill experiment with N=20 — identical to today's behavior -- The `SkillExperimentStrategy` is the default, so existing submissions work without changes -- `examples/sample_skill/metadata.yaml` does NOT need an `experiment` block (defaults apply) -- Add regression test fixture: run scaffold with old-format metadata, assert output matches current behavior - -## Commit Plan - -| # | Commit | Files | -|---|--------|-------| -| 1 | `feat: add ExperimentConfig, VariantSpec, CopySpec to schema` | `schemas.py`, `test_validate.py` | -| 2 | `feat: add ExperimentStrategy protocol and implementations` | `experiment.py`, `test_experiment.py` (new) | -| 3 | `refactor: scaffold.py — strategy-driven dirs, control/treatment` | `scaffold.py`, `test_scaffold.py` | -| 4 | `refactor: unify Dockerfile templates into Dockerfile.j2` | `Dockerfile.j2` (new), delete `.skilled.j2`/`.unskilled.j2`, `task.toml.j2` | -| 5 | `refactor: rename Tekton params/results to control/treatment` | `build-push.yaml`, `scaffold.yaml` | -| 6 | `docs: update terminology in plan, README, and analysis references` | `implementation_plan.md`, `README.md`, `scripts/analyze.py` | - -## Impact on Open PRs - -This refactor affects files already in open pull requests. The A/B generalization -should be implemented **after these PRs are merged** to avoid conflict, or as -follow-up commits on the relevant branches. - -### PR #1 — `APPENG-4903/phase-1-validation` ([link](https://github.com/RHEcosystemAppEng/ABEvalFlow/pull/1)) - -**Impact: HIGH** — schema is the foundation of the refactor. - -| PR file | A/B plan change | -|---------|-----------------| -| `abevalflow/schemas.py` | Add `ExperimentConfig`, `VariantSpec`, `CopySpec`, `ExperimentType` | -| `tests/test_validate.py` | Add tests for new schema models | -| `scripts/validate.py` | May need to validate `experiment` block if present | - -### PR #2 — `APPENG-4903/tekton-triggers-and-validate-task` ([link](https://github.com/RHEcosystemAppEng/ABEvalFlow/pull/2)) - -**Impact: LOW** — triggers and validate task are experiment-agnostic. - -| PR file | A/B plan change | -|---------|-----------------| -| `pipeline/tasks/validate.yaml` | No change needed (validates structure, not experiment type) | -| `pipeline/triggers/*` | No change needed (triggers are submission-agnostic) | -| `Docs/trigger_guide.md` | Minor terminology update (skilled/unskilled references) | -| `examples/sample_skill/metadata.yaml` | No change needed (defaults apply, no `experiment` block required) | - -### PR #3 — `APPENG-4904/phase-2-scaffolding` ([link](https://github.com/RHEcosystemAppEng/ABEvalFlow/pull/3)) - -**Impact: VERY HIGH** — scaffold, templates, and tests are the core of the refactor. - -| PR file | A/B plan change | -|---------|-----------------| -| `scripts/scaffold.py` | Major refactor: strategy pattern, `CopySpec`, control/treatment naming | -| `templates/Dockerfile.skilled.j2` | Delete — replaced by unified `Dockerfile.j2` | -| `templates/Dockerfile.unskilled.j2` | Delete — replaced by unified `Dockerfile.j2` | -| `templates/task.toml.j2` | Replace `variant == "skilled"` with `skills_dir` check | -| `templates/test.sh.j2` | No change (verified unaffected) | -| `pipeline/tasks/scaffold.yaml` | Rename results: skilled/unskilled → treatment/control | -| `tests/test_scaffold.py` | Major rename + new experiment config tests | - -### Branch `APPENG-4905/phase-3-build-push` (no PR yet) - -**Impact: MEDIUM** — param/result/step renames only. - -| Branch file | A/B plan change | -|-------------|-----------------| -| `pipeline/tasks/build-push.yaml` | Rename params/results/steps: skilled/unskilled → treatment/control | -| `Docs/implementation_plan.md` | Update terminology in Phases 3-6 | - -### Recommended Merge Order - -1. Merge PR #1, PR #2, PR #3, and phase-3 branch **as-is** (current skilled/unskilled naming) -2. Create a new branch `APPENG-XXXX/ab-testing-generalization` from `main` -3. Implement the A/B refactor as a single focused effort across all affected files -4. This avoids rebasing conflicts and keeps the current PRs reviewable - -Alternative: if PRs are slow to merge, the refactor can be applied directly on PR #3's branch (highest overlap) and the other PRs rebased after. - -## Files Changed Summary - -| File | Action | -|------|--------| -| `abevalflow/schemas.py` | Add `ExperimentConfig`, `VariantSpec`, `CopySpec`, `ExperimentType` | -| `abevalflow/experiment.py` | New — strategy protocol + `Skill`/`Model`/`ConfigDriven` implementations | -| `scripts/scaffold.py` | Refactor to use strategy, rename control/treatment, use `CopySpec` | -| `scripts/analyze.py` | Rename metric labels from skilled/unskilled to treatment/control | -| `templates/Dockerfile.j2` | New — unified template using `copy_pairs` | -| `templates/Dockerfile.skilled.j2` | Delete | -| `templates/Dockerfile.unskilled.j2` | Delete | -| `templates/task.toml.j2` | Replace `variant == "skilled"` with `skills_dir` check | -| `templates/test.sh.j2` | No changes (verified unaffected) | -| `pipeline/tasks/build-push.yaml` | Rename params/results/steps | -| `pipeline/tasks/scaffold.yaml` | Rename results | -| `tests/test_scaffold.py` | Rename + add experiment config and regression tests | -| `tests/test_validate.py` | Add ExperimentConfig/VariantSpec/CopySpec validation tests | -| `tests/test_experiment.py` | New — strategy unit tests | -| `Docs/implementation_plan.md` | Update terminology (targeted sections: Phase 4.4, 4.6, 5.1, 6) | -| `README.md` | Update terminology | diff --git a/Docs/implementation_plan.md b/Docs/implementation_plan.md index 35f89c33..ff97aa76 100644 --- a/Docs/implementation_plan.md +++ b/Docs/implementation_plan.md @@ -6,7 +6,7 @@ ## Overview -Build an automated, Tekton-orchestrated pipeline on OpenShift that accepts skill submissions, validates them, scaffolds skilled/unskilled container variants, builds images, runs Harbor evaluations via a custom OpenShift backend, and produces statistical reports comparing skilled vs. unskilled performance. +Build an automated, Tekton-orchestrated pipeline on OpenShift that accepts skill submissions, validates them, scaffolds treatment/control container variants, builds images, runs Harbor evaluations via a custom OpenShift backend, and produces statistical reports comparing treatment vs. control performance. ### Non-Goals @@ -22,7 +22,7 @@ This pipeline spans two repositories: | **[ABEvalFlow](https://github.com/RHEcosystemAppEng/ABEvalFlow)** (this repo) | Pipeline definitions, scripts, templates, config | Tekton YAML, Python scripts, Jinja2 templates, Harbor backend | | **[agentic-collections](https://github.com/RHEcosystemAppEng/agentic-collections)** | Skills, tasks, tests (post-evaluation) | Persona-based plugins (`rh-sre`, `rh-developer`, `ocp-admin`, etc.), 100+ skills | -The `tasks/` and `tasks-no-skills/` directories generated during scaffolding are **ephemeral workspace artifacts** — they exist only during a pipeline run, not as permanent directories in either repo. +The `tasks-treatment/` and `tasks-control/` directories generated during scaffolding are **ephemeral workspace artifacts** — they exist only during a pipeline run, not as permanent directories in either repo. ### Harbor Fork @@ -64,8 +64,7 @@ ABEvalFlow/ │ ├── analyze-report.yaml # Step 7 │ └── publish-store.yaml # Step 8 ├── templates/ # Jinja2 templates for scaffolding -│ ├── Dockerfile.skilled.j2 -│ ├── Dockerfile.unskilled.j2 +│ ├── Dockerfile.j2 │ ├── test.sh.j2 │ └── task.toml.j2 ├── scripts/ # Python scripts used by pipeline tasks @@ -192,8 +191,7 @@ Exit codes: `0` = pass, `1` = validation failure (with structured JSON error out **Goal:** Create templates that generate the correct Dockerfiles and supporting files. -- [ ] `Dockerfile.skilled.j2` — COPYs `skills/`, `docs/`, `tests/`, `supportive/`, and `instruction.md`. -- [ ] `Dockerfile.unskilled.j2` — COPYs `tests/`, `supportive/`, and `instruction.md` but **excludes** `skills/` and `docs/`. +- [x] `Dockerfile.j2` — Unified template using `copy_pairs` loop; COPYs strategy-determined directories plus common files (`tests/`, `supportive/`, `instruction.md`). - [ ] `test.sh.j2` — Entry script that runs the agent, then executes `test_outputs.py` and optional `llm_judge.py`. - [ ] `task.toml.j2` — Harbor task configuration. @@ -203,9 +201,9 @@ Exit codes: `0` = pass, `1` = validation failure (with structured JSON error out - Input: path to validated submission directory. - Output (ephemeral workspace artifacts, not permanent repo dirs): - - `tasks//` — skilled variant with rendered Dockerfile, test.sh, task.toml. - - `tasks-no-skills//` — unskilled variant. -- Renders templates with context from `metadata.yaml` and directory inspection (presence of `supportive/`, `docs/`, etc.). + - `tasks-treatment//` — treatment variant with rendered Dockerfile, test.sh, task.toml. + - `tasks-control//` — control variant (baseline). +- Renders templates with context from `metadata.yaml`, directory inspection, and experiment strategy (which determines copy specs per variant). ### 2.3 Scaffold Tekton Task (`pipeline/tasks/scaffold.yaml`) @@ -214,10 +212,10 @@ Exit codes: `0` = pass, `1` = validation failure (with structured JSON error out ### 2.4 Definition of Done -- [ ] Both variants produced with correct Dockerfile COPY directives. -- [ ] Skilled variant includes skills/docs; unskilled excludes them. -- [ ] `test.sh` and `task.toml` render correctly for both variants. -- [ ] Unit tests pass for `scaffold.py`. +- [x] Both variants produced with correct Dockerfile COPY directives via strategy-driven `copy_pairs`. +- [x] Treatment variant includes strategy-determined dirs (e.g., skills/docs for skill experiments); control excludes them. +- [x] `test.sh` and `task.toml` render correctly for both variants. +- [x] Unit tests pass for `scaffold.py`. --- @@ -225,11 +223,11 @@ Exit codes: `0` = pass, `1` = validation failure (with structured JSON error out ### 3.1 Build Task (`pipeline/tasks/build-push.yaml`) -**Goal:** Build both skilled and unskilled images and push to registry. +**Goal:** Build both treatment and control images and push to registry. - **Build tool constraint:** ADR Decision #5 specifies `docker buildx`. However, OpenShift clusters run CRI-O (not Docker) and do not provide a Docker daemon in pods. Using `docker buildx` inside unprivileged Tekton steps requires a Docker-in-Docker sidecar or socket mount, both of which require privileged access and contradict the security posture. **Buildah** (`buildah bud` + `buildah push`) is the standard rootless, daemonless alternative on OpenShift and runs in `ubi9` base images without privilege escalation. This constraint must be reconciled with ADR Decision #5 before implementation — likely by adopting Buildah for OpenShift. - Builds from the scaffolded directories. -- Tags: `//:skilled-` and `//:unskilled-`. +- Tags: `//:treatment-` and `//:control-`. - Push to **internal OpenShift registry** for evaluation (per ADR decision #6). - Quay promotion happens in Phase 6 (not here) to avoid double-push. @@ -238,23 +236,23 @@ Exit codes: `0` = pass, `1` = validation failure (with structured JSON error out - [ ] Create image pull/push secrets for Quay.io. - [ ] Configure OpenShift internal registry access for pipeline ServiceAccount. - [ ] Define image retention policy (default: 30 days on Quay for reproducibility). -- [ ] Add `latest-skilled` / `latest-unskilled` floating tags per skill for the monitoring pipeline. **Note:** Digest-based references remain the source of truth for reproducibility; floating tags are monitoring convenience only and may race under concurrent runs. +- [ ] Add `latest-treatment` / `latest-control` floating tags per skill for the monitoring pipeline. **Note:** Digest-based references remain the source of truth for reproducibility; floating tags are monitoring convenience only and may race under concurrent runs. ### 3.3 Image Reference Handoff The `build-push` Tekton task must emit two **results** for downstream consumption: -- `skilled-image-ref` — full digest-based reference (e.g., `registry/ns/skill@sha256:...`) -- `unskilled-image-ref` — same format +- `treatment-image-ref` — full digest-based reference (e.g., `registry/ns/skill@sha256:...`) +- `control-image-ref` — same format The `pipeline.yaml` wires these to the `harbor-eval` task: ```yaml params: - - name: skilled-image - value: "$(tasks.build-push.results.skilled-image-ref)" - - name: unskilled-image - value: "$(tasks.build-push.results.unskilled-image-ref)" + - name: treatment-image + value: "$(tasks.build-push.results.treatment-image-ref)" + - name: control-image + value: "$(tasks.build-push.results.control-image-ref)" ``` Use digest-based references (not mutable tags) between tasks to avoid tag mutation between push and eval. @@ -262,7 +260,7 @@ Use digest-based references (not mutable tags) between tasks to avoid tag mutati ### 3.4 Definition of Done - [ ] Both variants built and pushed to OpenShift internal registry. -- [ ] `skilled-image-ref` and `unskilled-image-ref` emitted as Tekton results (digest-based). +- [ ] `treatment-image-ref` and `control-image-ref` emitted as Tekton results (digest-based). - [ ] Push secrets functional. --- @@ -314,8 +312,8 @@ Additional requirements: ### 4.4 Trial Execution Configuration -- The `harbor-eval` Tekton task accepts `skilled-image-ref` and `unskilled-image-ref` as **params** wired from Phase 3 results. -- N = 20 attempts per variant (skilled + unskilled = 40 total sessions). +- The `harbor-eval` Tekton task accepts `treatment-image-ref` and `control-image-ref` as **params** wired from Phase 3 results. +- N = configurable attempts per variant (default 20, treatment + control = 40 total sessions). - Configure resource requests/limits per trial Pod. - LLM endpoint configured via environment variable — backend is agnostic to whether it points to LiteLLM, a direct API, or a self-hosted model. - Trial Pod timeout: configurable, with a global evaluation timeout. @@ -335,7 +333,7 @@ The pipeline ServiceAccount needs (prefer named Secrets for least-privilege wher ### 4.6 Definition of Done -- [ ] 40 trial Pods complete (20 skilled + 20 unskilled). +- [ ] Trial Pods complete (N per variant × 2 variants, default 40 total). - [ ] Cleanup verified — no stale Pods after evaluation. - [ ] Retry behavior validated for transient failures. - [ ] Unit tests pass with mocked K8s API. @@ -350,8 +348,8 @@ The pipeline ServiceAccount needs (prefer named Secrets for least-privilege wher **Goal:** Consume Harbor output and produce a statistical report. Metrics to compute: -- **Pass rate** per variant (skilled, unskilled). -- **Skills uplift (gap):** `pass_rate_skilled - pass_rate_unskilled`. +- **Pass rate** per variant (treatment, control). +- **Uplift (gap):** `pass_rate_treatment - pass_rate_control`. - **Statistical significance:** p-value via Fisher's exact test or chi-squared. - **Heatmap generation:** matplotlib/seaborn figures saved as PNG. - **LLM judge scores** (when `llm_judge.py` is present): include a qualitative score summary section. Define a schema for `llm_judge.py` output (JSON with `score`, `rationale`) to ensure `analyze.py` can reliably parse it. @@ -494,7 +492,7 @@ The pipeline and Harbor backend are agnostic — they pass LLM config as environ ### 8.5 Cost Controls & Observability -A single evaluation run consumes 40 LLM sessions (N=20 x 2 variants). Cost management is a first-class concern: +A single evaluation run consumes N × 2 LLM sessions (default N=20, 40 total). Cost management is a first-class concern: - [ ] Configure LiteLLM per-key budget limits (when using Vertex mode). - [ ] Implement pre-flight cost estimate: before launching Harbor, estimate token usage based on skill complexity and configured N. Log the estimate to the run summary to flag potential runaway cost before spend happens. diff --git a/Docs/workstreams_roadmap.md b/Docs/workstreams_roadmap.md new file mode 100644 index 00000000..c7f3135b --- /dev/null +++ b/Docs/workstreams_roadmap.md @@ -0,0 +1,149 @@ +# Workstreams Roadmap + +> Last updated: 2026-04-14 + +## Overview + +Four workstreams to complete the ABEvalFlow pipeline. WS1 is the critical path — it renames skilled/unskilled to treatment/control across the codebase and adds the A/B experiment framework. + +```mermaid +flowchart LR + WS1["WS1: A/B Generalization\n(APPENG-4932)"] --> WS2["WS2: Build & Push\n(APPENG-4905)"] + WS2 --> WS3B["WS3B: harbor-eval.yaml\n(APPENG-4906)"] + WS1 -.-> WS3A["WS3A: Harbor Doc Update\n(APPENG-4906)"] + WS1 -.-> WS4["WS4: Trigger Doc Update"] +``` + +--- + +## Current State (2026-04-14) + +| Item | Status | +|------|--------| +| PR #1 — Phase 1 validation (APPENG-4903) | Merged | +| PR #2 — Tekton triggers + validate task (APPENG-4903) | Merged | +| PR #3 — Phase 2 scaffolding (APPENG-4904) | Merged | +| PR #4 — Rename to ABEvalFlow | Merged | +| Branch `APPENG-4905/phase-3-build-push` | Stale — forked from `c98b547`, missing PRs #1-4. Abandoned. | +| Harbor OpenShift backend (`skills_eval_corrections`) | Feature-complete in fork, unit tested | + +--- + +## WS1: A/B Eval Flow Conversion (APPENG-4932) + +**Branch:** `APPENG-4932/ab-eval-flow-conversion` +**Plan:** [ab_testing_generalization_plan.md](./ab_testing_generalization_plan.md) + +Refactor the pipeline from hardcoded "skilled vs unskilled" to a general "treatment vs control" A/B framework. Skills remain the default experiment type. Adds support for model, prompt, and custom experiment types via a strategy pattern. + +### Execution Steps + +See the detailed commit plan in [ab_testing_generalization_plan.md](./ab_testing_generalization_plan.md). + +| Step | Commit | Key files | Tests | +|------|--------|-----------|-------| +| 1 | Schema: `ExperimentConfig`, `VariantSpec`, `CopySpec` | `abevalflow/schemas.py` | `tests/test_validate.py` | +| 2 | Strategy pattern | `abevalflow/experiment.py` (new) | `tests/test_experiment.py` (new) | +| 3 | Scaffold refactor | `scripts/scaffold.py` | `tests/test_scaffold.py` | +| 4 | Template unification | `templates/Dockerfile.j2` (new), delete old | existing tests cover | +| 5 | Tekton YAML renames | `pipeline/tasks/scaffold.yaml` | N/A (YAML only) | +| 6 | Docs + README terminology | `implementation_plan.md`, `README.md` | N/A | + +--- + +## WS2: Build & Push (APPENG-4905) + +**Branch:** `APPENG-4905/build-push-treatment-control` (to be created after WS1 merges) +**Depends on:** WS1 merged + +Recreate the build-push Tekton task from scratch on current `main` using treatment/control naming. The old `APPENG-4905/phase-3-build-push` branch is abandoned — it diverged from `c98b547` (before PRs #1-4) and would require a conflict-heavy rebase with no benefit. + +### What to build + +- `pipeline/tasks/build-push.yaml` — Buildah-based build and push for treatment/control images + - Params: `treatment-task-dir`, `control-task-dir`, `skill-name`, `commit-sha`, `registry-url`, `registry-namespace` + - Results: `treatment-image-ref`, `control-image-ref` (digest-based) + - Steps: `build-push-treatment`, `build-push-control` (rootless Buildah, `--storage-driver=vfs`) + - Image tags: `:treatment-`, `:control-` + - Namespace: `ab-eval-flow` +- `config/rbac.yaml` — RoleBinding for `system:image-builder` in `ab-eval-flow` namespace +- Update `Docs/implementation_plan.md` — Phase 3 checkboxes + +### Reference + +The old branch had a working `build-push.yaml` (109 lines) that can be used as a starting point, just with renamed params/results/steps and updated namespace. + +### Cleanup + +After this merges, delete the old `APPENG-4905/phase-3-build-push` branch (local and remote). + +--- + +## WS3: Harbor Backend (APPENG-4906) + +### WS3A: Harbor Handoff Doc Update + +**Can run in parallel** with any workstream. + +Update [harbor_openshift_backend.md](./harbor_openshift_backend.md) to match the actual implementation in `src/harbor/environments/openshift.py`: + +| Doc says | Reality | Action | +|----------|---------|--------| +| File: `openshift_environment.py` | `openshift.py` | Fix filename | +| `_build_and_push_image` is no-op only | Supports pre-built (`image_ref`) AND podman build | Document both modes | +| `readOnlyRootFilesystem: true` | Intentionally unset (many workloads need writes) | Update security section | +| RBAC: ConfigMaps, Secrets, ImageStreams | Only Pods + exec used | Narrow RBAC table | +| skilled/unskilled terminology | treatment/control after WS1 | Update naming | + +### WS3B: harbor-eval.yaml Tekton Task + +**Depends on:** WS2 merged (needs image ref handoff) + +New `pipeline/tasks/harbor-eval.yaml` in ABEvalFlow: +- Params: `treatment-image-ref`, `control-image-ref`, `n-trials`, `namespace` +- Runs `harbor run --env openshift --ek image_ref= --ek namespace=` +- Collects results to workspace/PVC + +### Investigation needed before WS3B + +Verify Harbor's CLI interface for trial count: `--runs`, `--n-trials`, config-based, or passed via `task.toml`. Check `harbor/cli/tasks.py` for the `--ek` help text. + +--- + +## WS4: Trigger Doc Update + +**Can run in parallel** with any workstream. + +Update [trigger_models_and_experiment_types.md](./trigger_models_and_experiment_types.md) based on discussion with Daniele Martinoli (2026-04-14): + +### Changes + +1. **Option 1 stays primary** — standalone submission repo (to be created, e.g. `RHEcosystemAppEng/ab-eval-submissions`) +2. **Clarify "ephemeral"** — the git submission is persistent as a git artifact, but it's not the final destination; it's an evaluation request. For skills there is code to contribute, but for agent/model/MCP comparisons the output is a decision (env var change, configuration), not a code contribution +3. **Option 2 enhancement — hybrid approach** — GH Action triggered by a PR label from admins, calling the same pipeline. The eval platform just needs the gitops submission repo to run, no matter how it's created. Skill-admins control their own trigger policy +4. **Admin-gating** — not all developers should trigger evaluations; admin label/approval gates the pipeline. This is on the skill-owner side, separate from the eval platform +5. **Two-role separation** — skill-admin (controls trigger policy, labels PRs) vs eval-platform (runs the pipeline from the submission repo) +6. **Non-code experiments** — agent compare, model compare, MCP eval are env-var/config changes, not repo contributions — reinforces why Option 1 is the natural universal fit + +### Scope + +Documentation update only. No code changes. The submission repo trigger (Option 1) is what we build. The PR-based hybrid (Option 2) is acknowledged and designed for, but not implemented now. + +--- + +## Execution Order + +1. **WS1** — A/B generalization (branch exists, start coding) +2. **WS4** — trigger doc update (can be done during WS1 PR review) +3. **WS3A** — harbor doc update (can be done during WS1 PR review) +4. **WS2** — build-push with treatment/control naming (after WS1 merges) +5. **WS3B** — harbor-eval.yaml task (after WS2 merges) + +## Jira Tickets + +| Ticket | Workstream | Status | +|--------|------------|--------| +| APPENG-4932 | WS1: A/B Eval Flow Conversion | In progress | +| APPENG-4905 | WS2: Build & Push Images | Blocked on WS1 | +| APPENG-4906 | WS3: Harbor OpenShift Backend | Partially done (fork complete, doc + Tekton task remain) | +| (none yet) | WS4: Trigger Doc Update | Not started | diff --git a/README.md b/README.md index 79bc8072..b3ba423b 100644 --- a/README.md +++ b/README.md @@ -6,12 +6,12 @@ Automated Tekton-orchestrated pipeline on OpenShift for evaluating AI skill subm 1. **Submit** — Push a skill directory to the submissions repo; a Tekton EventListener triggers the pipeline. 2. **Validate** — Checks structure, compiles test files, validates `metadata.yaml` schema. -3. **Scaffold** — Generates two container variants via Jinja2 templates: - - **Skilled** — includes the skill and reference docs. - - **Unskilled** — excludes them (baseline). +3. **Scaffold** — Generates two container variants via Jinja2 templates and an experiment strategy: + - **Treatment** — includes the experimental material (e.g., skills and reference docs for a skill experiment). + - **Control** — baseline without the experimental material. 4. **Build & Push** — Builds both images and pushes to the OpenShift internal registry. -5. **Evaluate** — Harbor runs N=20 attempts per variant (40 total) using a custom OpenShift backend. -6. **Analyze** — Computes pass rates, skills uplift (gap), statistical significance (p-value), and generates heatmaps. +5. **Evaluate** — Harbor runs N attempts per variant (default N=20, 40 total) using a custom OpenShift backend. +6. **Analyze** — Computes pass rates, uplift (gap), statistical significance (p-value), and generates heatmaps. 7. **Publish** — Stores reports, promotes passing images to Quay.io, and opens a PR to [agentic-collections](https://github.com/RHEcosystemAppEng/agentic-collections). ## Repository Structure diff --git a/pipeline/tasks/scaffold.yaml b/pipeline/tasks/scaffold.yaml index a48b3063..c4524598 100644 --- a/pipeline/tasks/scaffold.yaml +++ b/pipeline/tasks/scaffold.yaml @@ -5,8 +5,8 @@ metadata: namespace: ab-eval-flow spec: description: >- - Generates skilled and unskilled Harbor task directories from a validated - skill submission. Produces two output directories under the workspace for + Generates treatment and control Harbor task directories from a validated + submission. Produces two output directories under the workspace for downstream build tasks. params: - name: skill-dir @@ -27,10 +27,10 @@ spec: - name: source description: Workspace containing the cloned submissions repository results: - - name: skilled-task-dir - description: Path to the scaffolded skilled task directory - - name: unskilled-task-dir - description: Path to the scaffolded unskilled task directory + - name: treatment-task-dir + description: Path to the scaffolded treatment task directory + - name: control-task-dir + description: Path to the scaffolded control task directory steps: - name: clone-pipeline-repo image: registry.access.redhat.com/ubi9/python-311:latest @@ -56,17 +56,17 @@ spec: OUTPUT_DIR="$(workspaces.source.path)" cd "$PIPELINE_DIR" - pip install --quiet --no-cache-dir jinja2 pyyaml + pip install --quiet --no-cache-dir jinja2 pyyaml pydantic python scripts/scaffold.py "$SUBMISSION_DIR" "$OUTPUT_DIR" - SKILLED_DIR="$OUTPUT_DIR/tasks/$(params.skill-name)" - UNSKILLED_DIR="$OUTPUT_DIR/tasks-no-skills/$(params.skill-name)" + TREATMENT_DIR="$OUTPUT_DIR/tasks-treatment/$(params.skill-name)" + CONTROL_DIR="$OUTPUT_DIR/tasks-control/$(params.skill-name)" - echo -n "$SKILLED_DIR" > "$(results.skilled-task-dir.path)" - echo -n "$UNSKILLED_DIR" > "$(results.unskilled-task-dir.path)" + echo -n "$TREATMENT_DIR" > "$(results.treatment-task-dir.path)" + echo -n "$CONTROL_DIR" > "$(results.control-task-dir.path)" - echo "Skilled dir: $SKILLED_DIR" - echo "Unskilled dir: $UNSKILLED_DIR" - ls -la "$SKILLED_DIR" - ls -la "$UNSKILLED_DIR" + echo "Treatment dir: $TREATMENT_DIR" + echo "Control dir: $CONTROL_DIR" + ls -la "$TREATMENT_DIR" + ls -la "$CONTROL_DIR" diff --git a/scripts/scaffold.py b/scripts/scaffold.py index 05d3377e..7dacfb9d 100644 --- a/scripts/scaffold.py +++ b/scripts/scaffold.py @@ -56,7 +56,6 @@ def _build_template_context( "tags": tags, "has_supportive": (submission_dir / "supportive").is_dir(), "has_scripts": (submission_dir / "scripts").is_dir(), - "has_docs": (submission_dir / "docs").is_dir(), "has_llm_judge": has_llm_judge, # These were formerly ad-hoc dict reads from raw metadata; they are # not SubmissionMetadata fields (extra="forbid" rejects them), so the @@ -72,16 +71,7 @@ def _build_template_context( "storage_mb": metadata.storage_mb, } - ctx = strategy.customize_context(base_context, variant, submission_dir) - - # Map to old template name until templates are unified (Dockerfile.j2). - # task.toml.j2 uses `{% if variant == "skilled" %}` to emit skills_dir. - if ctx.get("skills_dir"): - ctx["variant"] = "skilled" - else: - ctx["variant"] = "unskilled" - - return ctx + return strategy.customize_context(base_context, variant, submission_dir) def _render_templates( @@ -89,10 +79,8 @@ def _render_templates( context: dict, ) -> dict[str, str]: """Render all templates for a variant, returning {filename: content}.""" - template_variant = context.get("variant", "skilled") - dockerfile_template = f"Dockerfile.{template_variant}.j2" return { - "Dockerfile": jinja_env.get_template(dockerfile_template).render(context), + "Dockerfile": jinja_env.get_template("Dockerfile.j2").render(context), "test.sh": jinja_env.get_template("test.sh.j2").render(context), "task.toml": jinja_env.get_template("task.toml.j2").render(context), } diff --git a/templates/Dockerfile.unskilled.j2 b/templates/Dockerfile.j2 similarity index 81% rename from templates/Dockerfile.unskilled.j2 rename to templates/Dockerfile.j2 index 51b4c633..61d58c62 100644 --- a/templates/Dockerfile.unskilled.j2 +++ b/templates/Dockerfile.j2 @@ -16,5 +16,6 @@ COPY supportive/ /workspace/supportive/ {% if has_scripts %} COPY scripts/ /workspace/scripts/ {% endif %} - -# Unskilled variant: no skills/ or docs/ — agent works without skill guidance +{% for src, dest in copy_pairs %} +COPY {{ src }}/ {{ dest }}/ +{% endfor %} diff --git a/templates/Dockerfile.skilled.j2 b/templates/Dockerfile.skilled.j2 deleted file mode 100644 index dcb34316..00000000 --- a/templates/Dockerfile.skilled.j2 +++ /dev/null @@ -1,24 +0,0 @@ -FROM registry.access.redhat.com/ubi9/python-311:latest - -USER 0 -RUN dnf install -y --quiet curl \ - && curl -LsSf https://astral.sh/uv/0.9.7/install.sh | env UV_INSTALL_DIR=/usr/local/bin sh \ - && dnf clean all -USER 1001 - -WORKDIR /workspace - -COPY instruction.md . -COPY tests/ /tests/ -{% if has_supportive %} -COPY supportive/ /workspace/supportive/ -{% endif %} -{% if has_scripts %} -COPY scripts/ /workspace/scripts/ -{% endif %} - -# Skilled variant: includes skills and docs for the agent -COPY skills/ /skills/ -{% if has_docs %} -COPY docs/ /workspace/docs/ -{% endif %} diff --git a/templates/task.toml.j2 b/templates/task.toml.j2 index 2de676c2..a886a368 100644 --- a/templates/task.toml.j2 +++ b/templates/task.toml.j2 @@ -23,6 +23,6 @@ build_timeout_sec = {{ build_timeout }} cpus = {{ cpus }} memory_mb = {{ memory_mb }} storage_mb = {{ storage_mb }} -{% if variant == "skilled" %} -skills_dir = "/skills" +{% if skills_dir %} +skills_dir = "{{ skills_dir }}" {% endif %}