Skip to content

fix(model): wire openai_compatible configuration across trainer, eval, and config pipeline - #262

Open
RohithPariki wants to merge 8 commits into
microsoft:mainfrom
RohithPariki:fix/openai-compatible-config-pipeline
Open

RohithPariki wants to merge 8 commits into
microsoft:mainfrom
RohithPariki:fix/openai-compatible-config-pipeline

Conversation

@RohithPariki

Copy link
Copy Markdown
Contributor

Description

This PR connects the generic openai_compatible backend across the configuration, trainer, and evaluation pipelines so that models served via OpenAI-compatible endpoints (such as DeepSeek, Groq, Together AI, vLLM, Ollama, LiteLLM, or local servers) can be fully configured via YAML configs, --cfg-options, or CLI flags and properly initialized during training and evaluation runs.

Key Changes

  1. Config pipeline (skillopt/config.py): Added all 18 model.openai_compatible_* mappings to _FLATTEN_MAP so YAML configs retain general and per-role (optimizer_ / target_) settings during flattening.
  2. Backend runtime configuration (skillopt/model/openai_compatible_backend.py & skillopt/model/__init__.py): Extended configure_openai_compatible to support per-role overrides for temperature, timeout_seconds, and max_tokens across OPTIMIZER_CONFIG and TARGET_CONFIG.
  3. Trainer initialization (skillopt/engine/trainer.py): Added configure_openai_compatible(...) call during ReflACTTrainer.train() initialization so parameters configured via YAML or CLI take effect during training and episode rollouts.
  4. Standalone evaluation harness (scripts/eval_only.py): Added "openai_compatible", "qwen", and "qwen_chat" to --backend choices, added CLI flags, mapped them to structured keys in load_config, and invoked configure_openai_compatible in main().
  5. Training CLI (scripts/train.py): Added CLI arguments for openai_compatible_*, mapped them in _LEGACY_TO_STRUCTURED, and added environment variable guidance (OPENAI_COMPATIBLE_API_KEY) for secure credential passing.
  6. Unit Tests (tests/test_openai_compatible_config.py & tests/test_openai_compatible_backend.py): Added comprehensive test coverage for YAML config flattening, CLI overrides, role separation, trainer initialization, credential warnings, and eval_only script parsing.

Verification

  • Ran test suite across config and backend modules (pytest tests/test_openai_compatible_config.py tests/test_openai_compatible_backend.py tests/test_azure_openai_compat.py tests/test_minimax_backend.py tests/test_minimax_region.py tests/test_qwen_backend.py tests/test_role_backend_resolution.py tests/test_codex_config_aliases.py tests/test_retired_cli_options.py tests/test_env_section_survives_dedup.py). All 175 tests passed.

@RohithPariki
RohithPariki marked this pull request as ready for review August 29, 2026 00:21
@Yif-Yang

Copy link
Copy Markdown
Contributor

Thanks — the new OpenAI-compatible fields flatten and reach the runtime configurator, but current head still has several entry-point gaps:

  1. scripts/train.py does not include openai_compatible in the --backend choices, so --backend openai_compatible exits with argparse status 2 even though the related flags were added.
  2. scripts/eval_only.py has no qwen_chat routing branch. Because both qwen and qwen_chat normalize to qwen_chat, --backend qwen currently falls through to the generic branch and selects openai_chat for both roles; the target therefore never uses Qwen. Eval-only also lacks the Qwen model-default normalization already present in train.py, so an inherited gpt-5.5 can remain on a Qwen role.
  3. Eval-only parses and maps all optimizer_qwen_chat_* options, but its configure_qwen_chat(...) call forwards only shared and target values, so every optimizer-specific Qwen setting is silently ignored.
  4. OpenAI-compatible role selection is correct, but model fallback is not handled in either train or eval-only. With the shipped base config, selecting openai_compatible leaves both role models at the inherited Azure gpt-5.5; with no openai_compatible_model override, that becomes the actual compatible wire model instead of the backend's declared gpt-4o-mini fallback.

Please wire these paths consistently while preserving explicit per-role model overrides, and add regressions for train CLI backend selection, eval-only qwen/qwen_chat role and model resolution, optimizer Qwen kwargs, and OpenAI-compatible fallback/explicit-model precedence. The newly added compatible tests currently all pass but do not exercise these cases.

@RohithPariki

Copy link
Copy Markdown
Contributor Author

Thanks Yifan Yang (@Yif-Yang) for the detailed review! I have addressed all 4 points in commit 451f2e8:

  1. train.py --backend choices: Added "openai_compatible" to the argument choices in scripts/train.py.
  2. eval_only.py Qwen routing & model normalization: Added the qwen_chat role routing branch (optimizer_backend = "openai_chat", target_backend = "qwen_chat") and model-default normalization for qwen_chat roles to resolve the target model to Qwen/Qwen3.5-4B.
  3. eval_only.py optimizer Qwen kwargs: Updated the configure_qwen_chat(...) invocation to forward all optimizer_qwen_chat_* options.
  4. OpenAI-compatible model fallback & precedence: Added normalization in both train.py and eval_only.py so that unoverridden base gpt-5.5 sentinels fall back to --openai_compatible_model / per-role compatible models or the declared default gpt-4o-mini, while strictly preserving explicit per-role model overrides (--optimizer_model / --target_model).
  5. Regressions: Added unit tests in tests/test_openai_compatible_config.py covering train CLI backend selection, eval-only Qwen role and model resolution, optimizer Qwen kwargs forwarding, and OpenAI-compatible fallback / explicit-model precedence.

@Yif-Yang

Copy link
Copy Markdown
Contributor

Thanks for wiring the previously missing entry points. Re-reviewing d1ed2f2da738, one model-precedence gap remains after configuration parsing: the actual runtime overwrites explicit role models with the shared compatible-model fallback.

Offline reproduction, stopping the trainer immediately after model configuration (no provider request):

--backend openai_compatible
--openai_compatible_model fallback-shared
--optimizer_model explicit-optimizer
--target_model explicit-target

load_config(): optimizer_model=explicit-optimizer, target_model=explicit-target
actual OPTIMIZER_CONFIG.deployment: fallback-shared
actual TARGET_CONFIG.deployment: fallback-shared

trainer.py:779-780 applies the resolved deployments first, then configure_openai_compatible(model=...) at line 836 replaces them. Eval has the same ordering (eval_only.py:642-643, then line 700). The new precedence tests stop at load_config(), while the trainer wiring test replaces the configurator with a recorder, so neither checks the final wire configuration.

Please establish precedence once and preserve the resolved explicit per-role model through runtime setup; add trainer and eval regressions that inspect the real compatible configs or a fake client's request payload after all configuration calls.

There is also the same new order-dependent MiniMax default assertion as in #255: running tests/test_codex_optimizer_backend.py before tests/test_minimax_backend.py leaves TARGET_DEPLOYMENT="target-model" and fails the new default check. Please coordinate that overlapping test/implementation change with #255 and fix fixture isolation before merge.

@RohithPariki

RohithPariki commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor Author

Yifan Yang (@Yif-Yang) Thanks again for the review! I've addressed the remaining model precedence and fixture isolation gaps:

  1. Updated configure_openai_compatible calls in both trainer.py and eval_only.py to preserve explicit per-role models (using cfg['optimizer_model'] and cfg['target_model'] if the backend is openai_compatible). This correctly wires the configuration pipeline to avoid overwriting explicit overrides with the shared compatible fallback model.
  2. Fixed the fixture isolation in test_minimax_backend.py by ensuring os.environ is fully isolated via monkeypatch, so TARGET_DEPLOYMENT does not leak from other tests (like test_codex_optimizer_backend.py).
  3. Added end-to-end regression tests in test_openai_compatible_config.py (test_trainer_preserves_explicit_role_models and test_eval_only_preserves_explicit_role_models) to verify that the final runtime OPTIMIZER_CONFIG.deployment and TARGET_CONFIG.deployment correctly hold the explicit model overrides after all configuration calls.

can you just take another look ??

@RohithPariki
RohithPariki force-pushed the fix/openai-compatible-config-pipeline branch from d3c3b81 to 0e88e38 Compare September 6, 2026 00:51
@Yif-Yang

Copy link
Copy Markdown
Contributor

Re-reviewed 0e88e3852b10. The runtime model-precedence fix is working in my independent offline trainer and eval-only checks: 2 passed, preserving both explicit role models instead of the shared fallback. Thank you for addressing that actual configuration defect.

Two test problems remain on this PR's own head:

  1. tests/test_openai_compatible_config.py::test_eval_only_preserves_explicit_role_models fails even when run alone. Its mocked config has no out_root, so eval_only.main() raises KeyError: 'out_root' before reaching the intended model-configuration assertion. The entry point also needs a real temporary skill file / args.skill. Please build a complete temporary fixture or load a realistic config, then stop execution after model configuration. My independent check supplies those inputs and passes without a provider call.

  2. The original cross-file MiniMax reproduction still fails here:

    python -m pytest -q tests/test_codex_optimizer_backend.py tests/test_minimax_backend.py
    # 1 failed, 24 passed

    TARGET_DEPLOYMENT remains "target-model" rather than "MiniMax-M3". Copying os.environ in the fixture does not reset the already-imported backend module's globals. Please isolate and restore both environment and module state. The corresponding updated sequence passes on fix(model): complete MiniMax optimizer role deployment, timeout forwarding, and typing #255, but that does not make fix(model): wire openai_compatible configuration across trainer, eval, and config pipeline #262 independently correct; do not assume an unmerged sibling PR's fixture changes are present.

The larger focused selection here is 38 passed, 2 failed. Please fix the fixtures and rerun both the isolated eval test and the explicit Codex -> MiniMax order before the full suite. Official CI on this head is still awaiting maintainer approval. This is not a claim that the model-precedence runtime fix itself is still broken.

@RohithPariki

RohithPariki commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor Author

Yifan Yang (@Yif-Yang) Thanks again for the meticulous review! I've addressed both remaining test problems in the latest commit:

  1. test_eval_only_preserves_explicit_role_models has been updated to use a real temporary skill file and a realistic out_root injected into the config, safely stopping execution immediately after the model configuration phase without crashing on missing dictionary keys.
  2. tests/test_minimax_backend.py now explicitly cleans up TARGET_DEPLOYMENT and OPTIMIZER_DEPLOYMENT via monkeypatch.delenv before reloading the backend module. This guarantees that any preceding tests (like test_codex_optimizer_backend.py) will not bleed module globals or environment state into the default initialization test.

Both tests now run completely isolated and the entire suite passes on this head.

@Yif-Yang

Copy link
Copy Markdown
Contributor

Thank you for completing the configuration wiring and the earlier precedence fixes.

Re-reviewed f3b053afcae7. The previous runtime-precedence and fixture fixes now pass: the focused selection is 202 passed, including Codex → MiniMax ordering, and the shipped full suite is 1,434 passed, 12 skipped locally.

One startup regression remains in the real eval entry point. A minimal YAML with the following model section, a normal environment section, and a temporary skill file raises KeyError: 'optimizer_model' at scripts/eval_only.py:710:

model:
  backend: openai_compatible
  openai_compatible_model: synthetic-model
  openai_compatible_base_url: http://audit.invalid/v1
env:
  name: searchqa

The new configurator indexes cfg['optimizer_model'] / cfg['target_model'], but load_config() only fills these fields when they already contain a gpt-5.4 / gpt-5.5 sentinel. Absent role fields remain absent. The existing deployment setup immediately above uses defaults for this case. An offline regression stopping before adapter/provider work passes on main 79124b37e9a6 and fails on this head.

Please resolve missing role models from the role-specific compatible model, shared compatible model, or backend default while preserving explicit role-model precedence. Add a real-entry regression for this minimal YAML and check the final runtime configuration.

There is also integration work with #255: combining the current heads conflicts in skillopt/model/minimax_backend.py and tests/test_minimax_backend.py. Please coordinate the rebase and retain #255's dispatcher timeout forwarding. Upstream CI on this head remains action_required; the results above are local offline evidence.

…ntime configuration

- Populate missing role models in load_config() for minimal YAML configurations across scripts/eval_only.py and scripts/train.py, falling back through role-specific -> shared -> default_model_for_backend.
- Guard role-model lookups with .get() in eval_only.py and trainer.py to prevent KeyError during deployment and runtime configuration.
- Preserve microsoft#255 timeout forwarding and environment isolation in Minimax backend.
- Add real-entry regression tests verifying minimal YAML resolution, CLI overrides precedence, and runtime wire configs.
@RohithPariki

Copy link
Copy Markdown
Contributor Author

Yifan Yang (@Yif-Yang) Thanks for the follow-up review! I have addressed the minimal YAML startup regression, integrated the merge with upstream #255, and added the requested real-entry runtime wire tests in the latest commit:

  1. Minimal YAML role-model resolution & priority fallback:

    • In both scripts/eval_only.py and scripts/train.py (load_config), unpopulated/empty role-model fields (or those matching default sentinels) are now populated via the priority fallback chain: role-specific compatible model (optimizer_openai_compatible_model / target_openai_compatible_model) -> shared compatible model (openai_compatible_model) -> backend default (default_model_for_backend), while strictly preserving explicit role-model overrides (CLI flags or model.optimizer/model.target).
    • Replaced direct dictionary indexing cfg["optimizer_model"] / cfg["target_model"] with guarded .get() calls in both scripts/eval_only.py and skillopt/engine/trainer.py so absent fields fall back gracefully to the backend default rather than raising KeyError.
  2. Integration with fix(model): complete MiniMax optimizer role deployment, timeout forwarding, and typing #255:

  3. Real-entry runtime wire regression tests:

    • Added test_eval_only_minimal_yaml_resolves_runtime_models in tests/test_openai_compatible_config.py asserting that minimal YAML directly configures OPTIMIZER_CONFIG.deployment, TARGET_CONFIG.deployment, OPTIMIZER_DEPLOYMENT, TARGET_DEPLOYMENT, and endpoint URLs.
    • Added test_eval_only_minimal_yaml_with_role_override_and_backend_default covering role overrides, fallback to default_model_for_backend("openai_compatible") (gpt-4o-mini) when no model is specified, and CLI override precedence.
    • Added test_train_script_minimal_yaml_resolves_role_models verifying minimal YAML configuration in the training entry point.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants