build: isolate optional runtime dependencies#1702
Conversation
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review Unresolved review comments identify potential bugs with the lockfile regeneration, TrainClient export behavior, and type compatibility when the renderers extra is installed. These substantive issues warrant human review. You can customize Macroscope's approvability policy. Learn more. |
![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: be386c1d8f
ℹ️ 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".
| modal = [ | ||
| "modal>=1.4.0", | ||
| ] |
There was a problem hiding this comment.
This change adds the modal extra and removes Modal/renderers from the base dependency set, but the tracked uv.lock still has modal, renderers, jinja2, textual, and regex under the base verifiers package and its provides-extras list still omits modal. In locked/frozen installs, uv sync --locked will continue installing the old heavy base dependencies, and verifiers[modal] is not represented in the lock, so the dependency isolation documented by this PR is defeated until the lockfile is regenerated.
Useful? React with 👍 / 👎.
macroscopeapp
Bot
dismissed
their stale review
Dismissing prior approval to re-evaluate 94ba449
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?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: 94ba449bcd
ℹ️ 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".
| "TrainClientConfig", | ||
| "resolve_client", | ||
| "EvalClient", | ||
| "TrainClient", | ||
| ] |
There was a problem hiding this comment.
When verifiers[renderers] is installed, existing code that imports TrainClient from the package (from verifiers.v1.clients import TrainClient) now fails because this package no longer defines or lazily resolves that name. The dependency isolation only needs to avoid importing renderers during base imports; add a __getattr__/lazy export with the missing-extra error instead of dropping the public export entirely.
Useful? React with 👍 / 👎.
| MultiModalData( | ||
| mm_hashes=renderer_mmd.mm_hashes, | ||
| mm_placeholders={ | ||
| modality: [ | ||
| PlaceholderRange(offset=p.offset, length=p.length) for p in ranges | ||
| ] | ||
| for modality, ranges in renderer_mmd.mm_placeholders.items() | ||
| }, | ||
| mm_items=renderer_mmd.mm_items, | ||
| ) |
There was a problem hiding this comment.
In runs that install verifiers[renderers], this conversion replaces the renderer's renderers.base.MultiModalData payload with the local fallback dataclass, so downstream training code that expects the renderer sidecar type or its methods no longer sees the original multimodal object. The fallback should only be used when renderers is absent; when the extra is installed, import and pass through the renderer's MultiModalData/PlaceholderRange types.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1ac2fb4. Configure here.
1 participant
Overview
Keep the base Verifiers install focused on default evaluation and platform paths while making Modal and renderer-backed training explicit opt-ins.
Details
verifiers[modal]and renderer-backed clients toverifiers[renderers]; keep renderers in the development group for repository integration coverage.TrainClientas a lazy package export and validate renderer configuration with the native renderer config union when the extra is installed.tomlimarker, and the obsolete regex workaround.User Impact
Default evaluation workflows continue to use the base package. Renderer-backed training requires
verifiers[renderers], and the Modal runtime requiresverifiers[modal].Note
Medium Risk
Install-time and import-path behavior changes for anyone relying on implicit
modal/renderers/textualin the base package; training and multimodal serialization paths need the renderers extra to match full behavior.Overview
Shrinks the default
verifiersinstall by movingmodal,renderers, and several unused direct deps (jinja2,textual, the oldregexpin, andtomlifor <3.11) out of coredependencies.verifiers[modal]andverifiers[renderers]are new optional extras;renderersstays in thedevgroup so repo CI still exercises renderer-backed paths.Renderer-backed training no longer loads at import time:
TrainClientis lazy-exported fromverifiers.v1.clients(clear error pointing atverifiers[renderers]),resolve_clientimports it only forTrainClientConfig, andRendererConfigdegrades todict[str, Any]whenrenderersis missing.v1 core types work without
renderers:MultiModalData/PlaceholderRangeare imported fromrendererswhen present, otherwise defined as lightweight dataclass fallbacks inverifiers.v1/types.py;graphandtraceimport those types fromverifiers.v1.typesinstead ofrenderers.base.uv.lockis regenerated so frozen installs match the slimmer default graph (e.g.textualand its markdown extras drop out of the base tree).User impact: plain
pip install verifiersstays eval/platform-focused; renderer training needsverifiers[renderers], Modal runtime needsverifiers[modal].Reviewed by Cursor Bugbot for commit 317e77d. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Make
renderers,modal,jinja2, andtextualoptional runtime dependenciesrenderers,modal,jinja2, andtextualfrom core dependencies in pyproject.toml; they are now available via extras/dependency groups.TrainClientis only imported on access, raising a clearImportErrorifrenderersis not installed.RendererConfigandTrainClientimports optional in verifiers/v1/clients/config.py, withTrainClientdeferred to resolution time.MultiModalDataandPlaceholderRangein verifiers/v1/types.py whenrenderersis unavailable, with graph.py and trace.py updated to import from there.renderersor related packages by default; code paths that require them will raiseImportErrorat access time rather than import time.Macroscope summarized 317e77d.