docs(gap-baseline): record SDK-parity (33-37) scoping, reclassify item 32 - #1746
Open
seonghobae wants to merge 3 commits into
Open
docs(gap-baseline): record SDK-parity (33-37) scoping, reclassify item 32#1746seonghobae wants to merge 3 commits into
seonghobae wants to merge 3 commits into
Conversation
…m 32
Read contextual-orchestrator's server.py (8744 lines), batch_routing.py,
and file_registry.py directly from protected main to build an evidence-
based gap list against the OpenAI SDK, rather than guessing scope.
Already implemented (broader than expected): chat/completions,
responses, completions (legacy), embeddings, models, files,
batch/embeddings, videos, images/generations, audio/{speech,
transcriptions,generations}, plus a non-standard rerank extension.
Genuinely missing: moderations, fine_tuning/jobs, audio/translations,
images/{edits,variations}, uploads, realtime (WebSocket -- flagged as
needing its own ADR before implementation, given the architectural
shift to a persistent connection). Assistants API family (assistants/
threads/runs/vector_stores) noted as missing but likely low priority,
since OpenAI itself is sunsetting it in favor of the already-implemented
Responses API.
Item 32 (batch scope) reclassified: batch_routing.py already has a
per-request RoutingPolicy dispatching to pg-llm-batch, so this needs
re-verification against the item's actual concern, not new
implementation from a blank slate.
Item 36 (S3-backed Files) confirmed as a real gap: current file_registry.py
is a provider-affinity proxy, not a self-hosted object-storage-backed
implementation.
Implementation deliberately deferred per this session's throttle
agreement: the GitHub Actions capacity crunch this document already
records (60-slot ceiling) was re-checked before this entry and is
unimproved (2154 queued / 2-4 in-progress) -- opening new feature PRs
now would add to it rather than help clear it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 41 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
|
||
| **Item 32 (batch endpoint scope) reclassified: not a blank-slate gap, needs re-verification instead.** `batch_routing.py` already implements a `RoutingPolicy` that decides sync-vs-batch **per request**, driven by request hints and KV-configured thresholds, dispatching batch-eligible requests to `pg-llm-batch` through an injected OpenAI-compatible `BatchAPIClient`. This may already satisfy item 32's concern ("should the batch endpoint only serve batch models") as a policy decision rather than a hard endpoint restriction — confirming that needs a closer read of `RoutingPolicy`'s actual thresholds and call sites, not new implementation. Left open for whoever picks this up next. | ||
|
|
||
| **Item 36 (S3-backed Files API) confirmed as a real gap.** `file_registry.py`'s current `FileOwner`/file-registry logic is a *provider-affinity* registry — it tracks which upstream provider a given gateway file id was proxied to, not a self-hosted store. No S3 (or other object-storage) integration exists. Item 36's "Files API can be self-implemented via S3" is not yet built. |
Contributor
There was a problem hiding this comment.
📝 Info: Files gap matches current behavior
The Files API proxies uploads upstream and persists only ownership metadata. No object payload store or S3 integration exists in the inspected implementation.
Was this helpful? React with 👍 or 👎 to provide feedback.
…fix xref Addresses three Devin review findings: - "Batch scope gap is misclassified": the entry claimed RoutingPolicy already gates the batch endpoint to batch-capable models and that eligible requests dispatch to pg-llm-batch through an injected BatchAPIClient. Re-read server.py/cost_router.py directly: /api/v1/batch_routing_jobs calls coordinator.submit_batch() directly, which never consults RoutingPolicy; and CostRoutingCoordinator defaults batch_backend to LocalBatchBackend unless a caller injects one, which server.py's one constructor call does not do (PgLlmBatchBackend is never instantiated outside tests/). Corrected the entry to keep item 32 open pending real verification, with exact file:line evidence for the corrected claim. - "External evidence lacks a revision pin": pinned the server.py citation to the exact commit inspected. - "Cross-repository reference is not linkable": `keyverse#103` -> the binding-convention form `ContextualWisdomLab/keyverse#103`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
Contributor
Author
|
Addressed three Devin review findings:
The "Files gap matches current behavior" comment was an info-only confirmation, no change needed. Generated by Claude Code |
|
|
||
| **Missing but likely low priority:** `/v1/assistants`, `/v1/threads`, `/v1/runs`, `/v1/vector_stores` (the Assistants API). OpenAI itself is sunsetting this family in favor of the Responses API, which this gateway already implements (`/v1/responses`). Building parity for a family the SDK's own vendor is deprecating is in tension with item 34's "if the SDK has it, implement it" principle — recorded as a judgment call to deprioritize, not a decision to never build it. | ||
|
|
||
| **Item 32 (batch endpoint scope) reclassified — corrected per Devin review, item stays open, not "policy already covers it."** `batch_routing.py` does implement a `RoutingPolicy` (`cost_router.py:106`, `self.policy = routing_policy or RoutingPolicy(...)`) that `CostRoutingCoordinator` uses to decide sync-vs-batch for *ordinary* `/v1/chat/completions`-shaped calls. But re-reading `server.py` and `cost_router.py` directly (at `main` commit `212ff437dc297613289dba2e6064ade9942e07d8`) shows this does **not** substantiate the original claim: the explicit `/api/v1/batch_routing_jobs` endpoint (`server.py:7443`) calls `coordinator.submit_batch(...)` (`:7453`) directly — `submit_batch` (`cost_router.py:970`) resolves the request's target model via `_resolve_batch_request` (only for ZDR-tagged requests) and hands off to `self.batch_backend.submit(...)`; neither path consults `self.policy`/`RoutingPolicy` at all, so nothing restricts this endpoint to batch-capable models. Separately, `CostRoutingCoordinator.__init__` (`cost_router.py:117-127`) defaults `self.batch_backend` to `LocalBatchBackend` unless a caller explicitly injects one, and `server.py`'s only constructor call (`server.py:5487`, `CostRoutingCoordinator(orchestrator)`) does not inject one — `PgLlmBatchBackend` (the class that wraps `pg_llm_batch.BatchAPIClient`) is never instantiated anywhere outside `tests/`. So "dispatching batch-eligible requests to pg-llm-batch through an injected BatchAPIClient" does not describe the server's actual default wiring. Item 32 remains open pending real verification of `/api/v1/batch_routing_jobs`'s actual model-resolution and backend wiring — not resolved by `RoutingPolicy`'s existence for a different code path. |
Contributor
There was a problem hiding this comment.
📝 Info: Batch classification matches pinned source
At the pinned revision, submit_batch bypasses RoutingPolicy; both production constructors leave batch_backend unset. Only tests instantiate PgLlmBatchBackend, supporting the open-gap classification.
Was this helpful? React with 👍 or 👎 to provide feedback.
seonghobae
added
documentation
ChatGPT Codex Connector
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Records the SDK-parity (backlog items 33-37) scoping pass and item 32 reclassification discussed with a peer session. Documentation only — no code changes, per this session's throttle agreement given the ongoing GitHub Actions capacity crunch (re-verified at commit time: 2154 queued / 2-4 in-progress, unimproved).
What's recorded
contextual-orchestratorvs. the OpenAI SDK, built by readingserver.py/batch_routing.py/file_registry.pydirectly rather than guessing.batch_routing.pyalready has non-trivial routing policy logic.Test plan
🤖 Generated with Claude Code