Open
Conversation
fe01647 to
7509d72
Compare
77a92af to
9fdb87c
Compare
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
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.
• Refactors agent config handling to centralize model configuration resolution in a dedicated utility.
• Introduces ResolvedModelConfig dataclass and resolve_model_config() to merge explicit params with ScenarioConfig defaults.
• Updates JudgeAgent and UserSimulatorAgent to use resolver, fixing precedence and edge cases (e.g., temperature=0.0).
• Adds comprehensive tests covering no-global-config, string default_model, ModelConfig defaults, falsy value handling, extra param merging, and edge cases.
Tests: WHEN / THEN breakdown
No global config (ScenarioConfig.default_config = None)
• WHEN resolve_model_config(model=“openai/gpt-4”)
▫ THEN returns model=“openai/gpt-4”, other fields None, extra_params={}
• WHEN resolve_model_config() with no model
▫ THEN raises ValueError(“Model must be configured…”)
• WHEN resolve_model_config(model=“openai/gpt-4”, api_base=“https://custom.com”, api_key=“sk-test”, temperature=0.5, max_tokens=1000, timeout=60, headers={“X-Test”: “value”})
▫ THEN explicit values are kept; extra_params={“timeout”: 60, “headers”: {“X-Test”: “value”}}
Global config: default_model as string
• GIVEN ScenarioConfig.default_config = ScenarioConfig(default_model=“openai/gpt-4”)
• WHEN resolve_model_config()
▫ THEN model=“openai/gpt-4”
• WHEN resolve_model_config(model=“anthropic/claude-3”)
▫ THEN model=“anthropic/claude-3” (explicit overrides)
• WHEN resolve_model_config(timeout=30)
▫ THEN extra_params={“timeout”: 30}
Global config: default_model as ModelConfig
• GIVEN ScenarioConfig.default_config = ScenarioConfig(default_model=ModelConfig(model=“openai/gpt-4”, api_base=“https://config.com”, api_key=“sk-config”, temperature=0.7, max_tokens=2000))
• WHEN resolve_model_config()
▫ THEN uses all defaults: model/openai/gpt-4, api_base/config.com, api_key/sk-config, temperature=0.7, max_tokens=2000, extra_params={}
• WHEN resolve_model_config(model=“anthropic/claude-3”, api_base=“https://override.com”, api_key=“sk-override”, temperature=0.1, max_tokens=500)
▫ THEN explicit overrides are applied for all fields
• WHEN resolve_model_config(temperature=0.3)
▫ THEN temperature=0.3 (explicit), other fields from config
Falsy values handling
• GIVEN defaults with temperature=0.7, max_tokens=2000
• WHEN resolve_model_config(temperature=0.0)
▫ THEN temperature=0.0 (explicit 0.0 overrides; not treated as falsy)
• WHEN resolve_model_config(max_tokens=0)
▫ THEN max_tokens=0 (explicit 0 overrides; not treated as falsy)
• WHEN resolve_model_config() with temperature unspecified
▫ THEN temperature uses config default (0.7)
• GIVEN config with api_base=“https://config.com”
• WHEN resolve_model_config(api_base=””)
▫ THEN api_base remains “https://config.com” (empty string treated as falsy for strings via or)
Extra params merging
• GIVEN config extra params: timeout=30, headers={“X-Config”: “config-value”}, max_retries=3 in ModelConfig
• WHEN resolve_model_config()
▫ THEN extra_params includes all config extras
• WHEN resolve_model_config(timeout=60, new_param=“value”)
▫ THEN explicit extra overrides: timeout=60; config extras preserved; new_param added
• GIVEN default_model is string (“openai/gpt-4”)
• WHEN resolve_model_config(custom=“param”)
▫ THEN extra_params == {“custom”: “param”} (no config extras to merge)
Edge cases
• GIVEN ModelConfig default temperature=0.0
• WHEN resolve_model_config(temperature=0.5)
▫ THEN temperature=0.5 (explicit overrides default 0.0)
• GIVEN ModelConfig default temperature=0.0
• WHEN resolve_model_config() without temperature
▫ THEN temperature=0.0 (uses ModelConfig default)
• WHEN resolve_model_config(model=“openai/gpt-4”, param=“value”)
▫ THEN no mutation of input kwargs; resolver succeeds without raising
Assumptions
• ScenarioConfig.default_config is mutated in tests and reset in setup/teardown.
• ModelConfig.model_dump(exclude_none=True) returns provider-specific extras to merge into extra_params.