Repository navigation
[Feat] Served OneJev multimodal decision backend (--model onejev) - #101
Conversation
HTTP client for OmniJev/OneJev via qev/flide TypeSafe-compatible /v1/systemone plus media and <image:N> state markers. Registers in build_model and the browser front; contract tests use a fake HTTP server (no GPU). Closes #88.
Birfy
left a comment
There was a problem hiding this comment.
Review (Birfy self-check — comment only, not an approve)
Overall: Looks good and ready to land once CI is green, with one should-fix on timeout/deadline semantics. Scope matches #88: served OneJev over TypeSafe-compatible /v1/systemone + media, browser + build_model only, stub-server contracts, docs/NOTICE/CHANGELOG attribution without claiming live GPU validation.
What is solid
- Wire shape matches upstream OneJev docs:
state.screen/image_N→<image:N>,media: [{type, data: data-URL}], stringinstructions+ flattened criteria viaomnijev_question(..., prompt="short"). Happy-path test pins this. - Error policy: no image →
MODEL_CALL_FAILED(same wording/status as in-processomnijev); empty/unsupported media types fail closed; transport / malformed answers / HTTP 500 mapped toMODEL_CALL_FAILED; missingONEJEV_URL/ bad timeout →MODEL_SERVICE_CONFIG_ERROR. - Provenance:
rawkeepsurl,request_id,served_by,server_timing,sent_imagesdigests and never the image bytes (covered by test). - Registration:
DECISION_MODEL_NAMES+ browser front only; tool/rails/decide/probeuntouched — same shape asomnijev/jev-vl-served. Registration + browse slot routing tests look right. - Docs / legal: configuration, decision-models (incl. smoke recipe), capabilities (registration vs unverified hardware), NOTICE + THIRD_PARTY_LICENSES (Apache-2.0, weights not vendored, PyPI
qevgotcha) are consistent with the code. - Default model id
OmniJev/OneJev1.6-4Bmatches current OneJev 1.6 naming (not the olderOneJev-4Bfrom the issue text).
Should-fix before merge
ONEJEV_TIMEOUT_Sdoes not match the documented “retry included” deadline.
docs/configuration.md/ the decision-models row say the timeout is a per-decision deadline that includes the one refused-connection / 502 / 504 retry.OneJevClient.decideinstead gives each attempt a fulltimeout_s(asyncio.wait_for(..., self._timeout_s)), so a retry can take ~2×.clockis accepted and stored but never used — looks like a half-port ofServedLayaClient’s shared-deadline pattern. Prefer: one monotonic deadline for the decision (and optionally share it with/health), subtract remaining on the retry, and either useclockor drop the unused parameter. Worth a contract test that a first transport blip then success still finishes under the configured budget (or at least that the second attempt’s wait_for uses remaining time).
Nits (non-blocking)
- No 503 + Retry-After path (Laya-served has it). Fine if intentional for qev/flide; just don’t document parity with laya-served retries beyond 502/504.
- Explicit
warm()fails hard on non-200 /health, while_warm_oncesoftens — fine, but a one-line comment on that split would help the next reader. - Optional: assert one successful retry after a single
ConnectErroror 502 (today only “two refuses → fail” is covered). docs/architecture.mdbrowser model lists still omitjev-vl-served/clm/laya-servedin a couple of places (pre-existing); addingonejevbesideomnijevis consistent with that incomplete list, but a follow-up tidy would help.
Tests
Contract suite covers happy path (media + marker), provenance, browser element flattening, missing image, transport after retry, malformed answers, HTTP 500, mapping helpers, from_env / build_model, and browser registration. Good enough for CI without a GPU.
CI
Spot-checked at review time: benchmarks already green; core / full still pending — merge only after those pass.
Not approving (own PR). Happy to re-check after the deadline fix lands on the branch.
hsliuustc0106
left a comment
There was a problem hiding this comment.
Review summary
Adds served --model onejev (HTTP /v1/systemone + media) with factory/browser registration, contract tests, and docs/NOTICE — scope matches #88. Overall risk is medium (new multimodal HTTP decision path, opt-in via ONEJEV_URL). CI is green on head b0a341b and the branch is mergeable, but documented timeout semantics do not match the code, so this is not ready to approve/merge yet.
Findings
[major] ONEJEV_TIMEOUT_S is not a shared decision deadline
- Where:
s1a/decision_models/onejev.py(OneJevClient.decide,clock);docs/configuration.md/docs/decision-models.md - Evidence: Docs say the timeout is a per-decision deadline with the one refused-connection / 502 / 504 retry included.
decideinstead wraps each attempt inasyncio.wait_for(..., self._timeout_s), so a retry can run ~2×.clockis stored and never read (half-port ofServedLayaClient). Same gap as landedclm.py, but new public surface should not copy it while advertising Laya-style inclusive wording. - Why it matters: Operators who set
ONEJEV_TIMEOUT_Sto bound step latency get a weaker guarantee than documented; async/timeouts are a first-class review focus here. - Suggested fix: One monotonic deadline per decision (use
clockor drop it); on retry useremaining = deadline - clock()for both httpx timeout andwait_for. Optionally share the budget with/health. Add a contract test that a first transport blip then success stays under the configured budget (or that attempt 2’s wait uses remaining).
Questions
None.
No other blockers found: wire shape, no-image refusal, provenance digests, registration (browser + build_model only), and stub-server tests look sound. CI on this head: core/full/benchmarks green.
| started = time.perf_counter() | ||
| post = self._client.post(f"{self.url}/v1/systemone", json=body, headers=headers, timeout=self._timeout_s) | ||
| try: | ||
| response = await asyncio.wait_for(post, self._timeout_s) |
There was a problem hiding this comment.
[major] Per-attempt timeout vs documented shared deadline
docs/configuration.md / the decision-models row say ONEJEV_TIMEOUT_S includes the one refused-connection / 502 / 504 retry. This wait_for(..., self._timeout_s) (and the matching httpx timeout=) restarts the full budget on every attempt, so a retry can take ~2×. clock is accepted above and unused — prefer ServedLayaClient-style remaining budget (or drop clock and rewrite the docs to per-attempt). Add a contract test that a first transport blip then success finishes under the configured budget.
[Fix] Share OneJev's HTTP deadline across retries
hsliuustc0106
left a comment
There was a problem hiding this comment.
Review summary
Re-checked tip 8a3d9ac after #105 landed the shared-deadline fix. Prior major finding is resolved: OneJevClient.decide now uses one monotonic deadline via clock, passes remaining budget to httpx/wait_for on retry, and contract tests cover remaining budget, exhausted budget, and stalled-retry cancel. Docs (ONEJEV_TIMEOUT_S inclusive wording) match the code. Scope still matches #88; CI green on this head (core/full/benchmarks); branch mergeable. No remaining blockers — approving.
Findings
None.
Light impact
- Impact: new
onejevpath + factory/browse registration, docs/NOTICE; first-hop callers arebuild_model/ browser front only. - Risk: medium (new opt-in multimodal HTTP decision path).
- Missing tests: none obvious for the deadline fix.
Required human contributor answers
Per the repo owner's (Danyang, @Birfy) standing instruction, these are filled from the issue (#88) instead of waiting for separate human answers.
What outcome are you trying to achieve?
Answer: Agents can call a served OneJev multimodal System One model over HTTP (
--model onejev) for screenshot + textchoice/noul, without cloning OmniJev into the process ([Feat] Add a served OneJev multimodal decision backend (System One + media) #88).What is the highest-leverage variable for that outcome?
Answer: Whether the HTTP body matches OneJev/qev's documented TypeSafe-compatible
/v1/systemoneplusmedia/<image:N>convention, with clean refusal when the observation has no image.How will this work move that variable rather than create busywork?
Answer: It adds a dedicated served adapter (like
laya-served/ CLM) that posts observation + questions, records provenance (url,request_id,served_by), and is covered by fake-HTTP contract tests so CI does not need a GPU.Who is the customer or user, and what do they actually need?
Answer: People running browser agents who want OneJev vision decisions against a local
qev/flideserver without the in-process OmniJev path.After answering the four questions: is a PR needed at all to achieve this outcome?
Human decision and rationale (or explicit source reference): Yes; #88 proposes this adapter as the scope of one PR.
For a bug fix: link actual observed failure evidence.
Evidence: Not a bug fix: a new served backend (#88).
Why
system1-agents has in-process
--model omnijevbut no served OneJev/OmniJev worker client. OmniJev org's OneJev speaks TypeSafe/v1/systemonewith amediafield. Closes #88.How
Start with
s1a/decision_models/onejev.py, thenfactory.py/browse.pyandtests/test_decision_models_onejev.py.--model onejev→OneJevModel.from_env():ONEJEV_URL(required), optionalONEJEV_MODEL(defaultOmniJev/OneJev1.6-4B),ONEJEV_API_KEY,ONEJEV_TIMEOUT_S(default 30).statewith<image:N>/screen,mediaas PNG/JPEG/WebP data URLs, questions mapped via OmniJev's short prompt (string instructions, flattened element rows).MODEL_CALL_FAILED(OmniJev policy). Transport errors / malformed answers mapped the same way as other served clients.rawkeepsserved_by,url,request_id,server_timing,sent_imagesdigests (never image bytes).DECISION_MODEL_NAMESand browser front only (not tool/rails/decide), same shape asomnijev/jev-vl-served.What
ONEJEV_URL,ONEJEV_MODEL,ONEJEV_API_KEY,ONEJEV_TIMEOUT_Sindocs/configuration.md.docs/decision-models.md(backend row + optional smoke recipe),docs/capabilities.md(registration vs unverified hardware),docs/agents.md,docs/architecture.md.Verification
uv run ruff format --check . && uv run ruff check . && uv run ty check: all cleanuv run pytest -qandscripts/smoke.sh: 1339 passed, 132 skipped; smoke OKCHANGELOG.mdand the docs say what the code does nowOffline tests cover happy path (media + marker), missing image, transport error (after retry), malformed answers, HTTP 500,
from_env/build_model, and browser registration.Demo / evidence
issue-88-onejev-served@b0a341b; no System1-Omni involved (talks to upstream OneJevqev/flide).