Repository navigation
fix(args): broadcast environment options across models - #682
linjiaye929 wants to merge 2 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38d5451323
ℹ️ 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".
| `INFINITY_MODEL_ID`. Like the v2 CLI, singleton options (including defaults) | ||
| apply to every model. A list with multiple option values must have exactly one | ||
| value per model; other lengths raise `ValueError`. |
There was a problem hiding this comment.
Qualify the length guarantee for omitted options
When two models are configured through this documented factory with INFINITY_DEVICE="cuda" and INFINITY_DEVICE_ID="0;1", this guarantee is false: EngineArgs.from_env() never passes MANAGER.device_id to AutoPadding, while the EngineArgs.device_id default factory reads MANAGER.device_id[0] for every constructed object. The second value is therefore silently ignored (and a mismatched device-ID list is never rejected), which can place every engine on GPU 0; either pad this field too or explicitly limit the guarantee to the variables the factory handles.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed: the new documentation overstated the scope. On both unchanged main and this branch, a CPU/debugengine configuration with two models and INFINITY_DEVICE_ID=0;1 produces device IDs [[0], [0]]; 0;1;2 is also accepted. These checks exercised configuration construction only, without GPU inference.
aab9569 qualifies the documentation and from_env() docstring to the fields actually passed through the factory, explicitly notes the first-entry device-ID behavior and omitted vector-cache option, and points to explicit EngineArgs construction for those fields. Executable behavior is unchanged. All 19 focused tests still pass; formatting, lint, spelling and diff checks pass.
Summary
EngineArgs.from_env()useszip_longestto align model IDs and option lists. With two semicolon-separated model IDs and ordinary singleton/default options, the second configuration receivesNonevalues and raisesKeyError(None)while resolving its engine.The v2 CLI already broadcasts singleton options and rejects other lengths that do not match the number of models. This change applies those same length rules to the Python environment factory:
AutoPaddingandtyper_option_resolvehelpers to a dependency-free shared module.infinity_emb.cli.EngineArgs.from_env()for its existing supported fields.No inference, engine-iterable consumption, cache, embedding dimensions, or additional environment-parameter support is changed.
Reproduction
Set these before importing Infinity:
Then call
EngineArgs.from_env(). On unmodified main it raisesKeyError(None); with this change it produces two configurations. Equivalent values already work through the CLI's existingAutoPaddinghelper.Validation
Linux, Python 3.12.14, NumPy 1.26.4, CPU-only PyTorch 2.8.0, sentence-transformers 3.3.1, pytest 8.4.2, Typer 0.12.5.
From
libs/infinity_emb, withDO_NOT_TRACK=1 HF_HUB_OFFLINE=1 TRANSFORMERS_OFFLINE=1:040269bc: 12 failed, 6 passed, with the new-module import-identity check excluded as inapplicable to baseline.debugengineinstances and exercise native batching, embedding and shutdown without model downloads or inference mocks. This is not pretrained-model inference or a quality benchmark.Adjacent checks:
python -m pytest tests/unit_test/test_args_from_env.py tests/unit_test/test_args.py \ tests/unit_test/test_engine.py::test_async_api_debug \ tests/unit_test/test_engine.py::test_args_between_array_and_engine_same \ tests/unit_test/test_sync_engine.py::test_sync_engine \ tests/unit_test/test_sync_engine.py::test_sync_engine_double -q python -m pytest tests/unit_test/test_cli.py -k 'help or wrong_batch_size or weird' -qruff check ., changed-file formatting/codespell, andgit diff --checkpass.mypy infinity_embreports the same 16 missing-optional-dependency errors in 10 files on this branch and unchanged main; no new diagnostics.make precommitwere not validated in this minimal environment.Hosted CI status
For head
38d545132346e85c187937f5553e768d3c9c7421, the upstream workflow is completed withaction_required. It has zero jobs, zero check runs and zero commit statuses. The reason is unconfirmed; no hosted tests have executed, so this is neither a hosted test pass nor a demonstrated code-test failure. The validation above is local CPU/debug-engine coverage only.Checklist
AI assistance was used to prepare this contribution.