COE-227: Canonical Data Store and Collection Pipeline#5
COE-227: Canonical Data Store and Collection Pipeline#5
Conversation
Implements COE-227: - Database schema with 9 canonical tables (providers, harness_profiles, variants, experiments, task_cards, sessions, requests, metric_rollups, artifacts) - Repository layer with 9 repositories for all entities - Session service for lifecycle management - LiteLLM collector and normalizer for request ingestion - Prometheus collector and rollup computations - Unit and integration test suites Key design decisions: - SQLAlchemy async ORM with PostgreSQL UUID types - Foreign key constraints for referential integrity - Unique constraints on session_id, proxy_key_alias, litellm_call_id - Percentile calculations with empty list handling for NumPy 2.x Tests: 40 unit tests + 17 integration tests all pass
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82763a0ab8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| session_rollups = await self.session.execute( | ||
| select(MetricRollupModel).where( | ||
| MetricRollupModel.scope_type == RollupScopeType.SESSION, | ||
| MetricRollupModel.scope_id.in_([s.session_id for s in sessions]), | ||
| ) |
There was a problem hiding this comment.
Variant-level median_latency_ms/p95_latency_ms are built from one precomputed median per session, so sessions with very different request counts are weighted equally. In a common case like 100 fast requests in one session and 1 slow request in another, this reports a 50/50 blend instead of the true request-level distribution, which will skew the core variant comparisons this service is meant to produce.
Useful? React with 👍 / 👎.
| session_id=UUID(session_id) if session_id else None, | ||
| experiment_id=UUID(correlation_keys["experiment_id"]) if correlation_keys.get("experiment_id") else None, | ||
| variant_id=UUID(correlation_keys["variant_id"]) if correlation_keys.get("variant_id") else None, | ||
| provider_id=UUID(correlation_keys["provider_id"]) if correlation_keys.get("provider_id") else None, |
There was a problem hiding this comment.
When a LiteLLM row is correlated only through proxy_key_alias, _resolve_session() can recover the session, but this constructor still leaves experiment_id and variant_id null because it only trusts raw tags. compute_experiment_rollups() later filters on RequestModel.experiment_id, so alias-only traffic disappears from experiment totals and latency aggregates even though the matched session already tells us which experiment/variant it belongs to.
Useful? React with 👍 / 👎.
| if await self.request_repo.exists_by_litellm_call_id( | ||
| self.session, litellm_call_id | ||
| ): |
There was a problem hiding this comment.
This existence check does not make ingestion idempotent under concurrency. If two collector workers or retries ingest the same litellm_call_id at the same time, both can observe False here and then race into RequestRepository.create(), where the loser hits the unique constraint on commit and fails the collection run instead of treating the duplicate as a no-op.
Useful? React with 👍 / 👎.
| ] | ||
|
|
||
| [project.scripts] | ||
| bench = "cli.main:cli" |
There was a problem hiding this comment.
bench script at a real CLI entrypoint
Installing this package will register bench, but the entrypoint targets cli.main:cli and this commit only adds src/cli/__init__.py; there is no src/cli/main.py or exported cli object anywhere in the repo. Anyone following the documented pip install -e . flow will get an import error before the CLI can start.
Useful? React with 👍 / 👎.
1 participant
Implements the canonical benchmark database, collectors, normalization, and rollups for COE-227.
Changes
Test Results
All 57 tests pass (40 unit + 17 integration)
Acceptance Criteria
All 15 acceptance criteria met as documented in the Linear issue.