feat: implement MLflow Observer (Phase C)

[Aliciapet11]

Summary

• Add MLflowObserver that auto-logs evaluation results to MLflow when MLFLOW_TRACKING_URI is set
• One experiment per submission, one run per pipeline execution
• Logs metrics (uplift, pass rates, gate scores, token usage), parameters, tags, and artifacts
• Store task points to Guy's MLflow instance
• Add mlflow as optional dependency and to store task pip install
• Include MLflow dashboard metrics proposal doc

Context

Phase C of APPENG-5370. Tested successfully against Guy's MLflow instance — experiments visible with real metrics, artifacts (scorecard.json, report.md), and run comparison.

Validation

• All 1121 tests pass locally
• CI ASE pipeline passed on cluster with MLflow logging confirmed
• A2A pipeline runs passed
• MLflow experiment abevalflow/hello-world-full visible with metrics, tags, and artifacts

Test plan

• Existing tests pass (1121/1121)
• New MLflow observer tests (11 tests) — mock-based
• CI ASE pipeline passes with MLflow enabled
• Experiments visible in MLflow UI with correct metrics and artifacts
• Observer gracefully skips when MLFLOW_TRACKING_URI not set
• Observer gracefully skips when mlflow package not installed

[GuyZivRH]

Consolidated PR Review: #64 — MLflow Observer (Phase C)

PR: #64
Author: Aliciapet11 (apetruni)
Branch: feat/mlflow-observer → main
HEAD: e5b2029f
Size: +1,915 / −60 · 9 files · 2 commits
CI: PASS (~1121 tests; cluster validation reported by author)
Ticket: APPENG-5370 Phase C
Consolidated from: krvwm_, qazwx_, rkdwe_, rkwmz_, tuirh_ (2026-08-04)

---

Verdict

Request changes before merge.

Consensus: the observer pattern, optional [observability] extra, failure isolation, mock tests, and experiment/run model are the right Phase C design and nearly ready. Every detailed review flags the same hard blocker: store.yaml hardcodes MLFLOW_TRACKING_URI to guy-ziv-evalflow. Most also require fixing misleading gates_passed / gates_failed metrics. Softer “approve” reviews still ask to parameterize the URI before treating this as production-default.

---

What this PR is

Component	Change
abevalflow/observability/mlflow_observer.py	New observer — params, metrics, tags, artifacts
abevalflow/db/observer.py	Discover when MLFLOW_TRACKING_URI set; pass report_dir / pipeline_run_id
scripts/store_results.py	Notify observers after DB persist
pipeline/tasks/post/store.yaml	Install mlflow; set tracking URI
pyproject.toml + CI + uv.lock	Optional mlflow>=2.10; CI installs .[dev,observability]
tests/test_mlflow_observer.py	~11 mock-based tests
Docs/mlflow_dashboard_proposal.md	Dashboard / metric catalog for Phase C/D

Model: one MLflow experiment per submission ({prefix}/{submission_name}), one run per pipeline execution (run name = Tekton PipelineRun id).

---

What looks good (consensus)

1. Fills the long-standing observer stub — discover_observers() actually loads MLflow when URI is set.
2. Non-blocking — observer / helper failures warn; store/pipeline keep going.
3. Optional dependency — core install stays lean; store/CI pull mlflow when needed.
4. Useful telemetry — uplift, pass rates, p-values, scorecard artifacts, certification, token checkpoint.
5. Solid unit tests — experiment create/reuse, metrics, tags, artifacts, discover empty vs enabled.
6. Run naming — prefers Tekton pipeline_run_id over DB UUID for cross-reference.
7. CI green + reported live MLflow UI validation.
8. Proposal doc — good stakeholder artifact for Phase D / Grafana follow-on.

---

Must fix (blocking)

1. Hardcoded personal-namespace MLflow URI

# pipeline/tasks/post/store.yaml
- name: MLFLOW_TRACKING_URI
  value: "http://abevalflow-mlflow.guy-ziv-evalflow.svc.cluster.local:5000"

Always-on, tenant-specific DNS in shared task YAML. Couples every store consumer to one namespace; conflicts with shared ab-eval-flow MLflow manifests called out in reviews.

Fix: Tekton param mlflow-tracking-uri (default "") and/or Secret/ConfigMap; only export when set. Align with AEH #61 enable pattern if present. Document MLFLOW_TRACKING_URI + MLFLOW_EXPERIMENT_PREFIX in ops docs.

2. Misleading gates_passed / gates_failed

def _count_gates(...):
    return sum(1 for scan in result.security_scans if scan.passed == passed)

Counts security scans, not unified scorecard gates. Proposal doc and dashboard charts imply scorecard semantics.

Fix: Read scorecard.json gates_passed / gates_failed, or rename metrics to security_scans_passed / security_scans_failed.

3. Per-gate metric key collisions (from deeper review)

Logging gate_score_{gate_name} overwrites when multiple gates share gate_name (e.g. "security") and differ by policy_key.

Fix: Use policy_key or {gate_name}_{policy_key} (sanitize for MLflow). Same for findings metrics.

---

Should fix

#	Issue	Ask
4	MLflow only runs when Postgres store runs (DATABASE_URL early exit; notify after DB)	Document coupling, or observer-only path when URI set and DB absent
5	Protocol still (result, run_id) only; TypeError retry masks real TypeErrors	Extend Protocol / **kwargs; use inspect.signature or match “unexpected keyword” only
6	“Lazy import” commit still has module-level import mlflow in observer module	OK if only imported via guarded discover; true lazy = import inside methods; don’t import from observer.py at top level
7	Unconditional pip install mlflow in store	Install only when URI/enable set, use .[observability], bake into image, or consider mlflow-skinny
8	Proposal vs code: recommendation / highest_certification as tags not metrics; combination_mode not logged	Sync doc or implement
9	Two MLflow layers: AEH evaluate-time (#61) vs store fleet observer	Document experiment naming so operators don’t confuse trial traces with scorecard runs
10	Experiment create race	Catch already-exists and re-get
11	No auth/TLS wiring	Document in-cluster open MLflow assumption
12	Large uv.lock churn	Expected; verify intentional / no conflict with skinny path
13	Plan doc still says MLflowObserver (future)	Update results_persistence_and_observability_plan.md on merge
14	Missing tests: malformed JSON, kwargs reaching observer, TypeError fallback	Add a few cases

---

Minor / nits

• Duplicate eval_engine as param and tag
• Findings metric naming (gate_findings_* vs proposal security_findings_count)
• Integer trial counts as metrics — fine
• PR body should stop saying store “points to Guy’s MLflow” after param fix

---

Merge bar

• Remove hardcoded guy-ziv-evalflow URI; param/secret + empty default (opt-in)
• Fix or rename gates_passed / gates_failed
• Disambiguate gate metric keys with policy_key
• Update Protocol + safer notify_observers
• Document DB+URI coupling (or support MLflow without DB)
• Align proposal / observability plan docs with implementation
• (Nice) AEH vs store MLflow layering note; experiment race; conditional pip install

---

Sources

File	Stance	Notable unique findings
krvwm_pr64_mlflow_observer.md	Approve w/ minor fixes	DB coupling; AEH #61 layering; mlflow-skinny; plan doc still “future”; lazy-import commit nuance
qazwx_pr_64_review.md	Approve	High-level pros; bracket install .[observability] suggestion
rkdwe_pr64_review.md	Request changes	Hardcoded URI; wrong gate metrics; TypeError mask; conditional pip
rkwmz_pr64_review.md	Approve w/ minor changes	Param template; malformed JSON test gap; proposal naming diffs
tuirh_pr_64_review.md	Request changes	gate_name collisions; Protocol drift; experiment race; auth/TLS

Authoritative blockers: hardcoded tracking URI, misleading gate-count metrics, gate metric key collisions.

[Aliciapet11]

All 3 blocking items fixed:

• Item 1 — Removed hardcoded guy-ziv-evalflow URI. Added mlflow-tracking-uri task parameter with empty default (opt-in). Observer only activates when set.
• Item 2 — gates_passed/gates_failed now read from scorecard.json fields instead of counting security scans.
• Item 3 — Gate metric keys now use policy_key (e.g. gate_score_harbor, gate_score_cisco) instead of gate_name to avoid collisions when multiple gates share the same name.