-
Notifications
You must be signed in to change notification settings - Fork 6
add support for model parameters passing #98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -107,7 +107,7 @@ | |||||||||||||
| exclude_from_context: bool = False, | ||||||||||||||
| max_turns: int = DEFAULT_MAX_TURNS, | ||||||||||||||
| model: str = DEFAULT_MODEL, | ||||||||||||||
| model_settings: ModelSettings | None = None, | ||||||||||||||
| model_par: dict = {}, | ||||||||||||||
|
||||||||||||||
| run_hooks: TaskRunHooks | None = None, | ||||||||||||||
| agent_hooks: TaskAgentHooks | None = None): | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -130,10 +130,11 @@ | |||||||||||||
|
|
||||||||||||||
| # https://openai.github.io/openai-agents-python/ref/model_settings/ | ||||||||||||||
| parallel_tool_calls = True if os.getenv('MODEL_PARALLEL_TOOL_CALLS') else False | ||||||||||||||
| model_settings = ModelSettings( | ||||||||||||||
| temperature=os.getenv('MODEL_TEMP', default=0.0), | ||||||||||||||
| tool_choice=('auto' if toolboxes else None), | ||||||||||||||
| parallel_tool_calls=(parallel_tool_calls if toolboxes else None)) | ||||||||||||||
| model_params = {'temperature' : os.getenv('MODEL_TEMP', default = 0.0), | ||||||||||||||
|
||||||||||||||
| model_params = {'temperature' : os.getenv('MODEL_TEMP', default = 0.0), | |
| model_params = {'temperature' : os.getenv('MODEL_TEMP', default=0.0), |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The os.getenv('MODEL_TEMP', default=0.0) call returns a string, not a float. Environment variables are always strings, so this will pass "0.0" (a string) to ModelSettings instead of the numeric value 0.0. This should be float(os.getenv('MODEL_TEMP', default='0.0')) or use default=0.0 only after converting the environment variable value to a float.
| model_params = {'temperature' : os.getenv('MODEL_TEMP', default = 0.0), | |
| model_params = {'temperature' : float(os.getenv('MODEL_TEMP', default='0.0')), |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent spacing around the assignment operator. The code uses 'temperature' : os.getenv(...) with spaces around the colon, which is not standard Python style. According to PEP 8, dictionary keys should not have spaces around the colon. This should be 'temperature': os.getenv(...) (and similarly for the other keys).
| model_params = {'temperature' : os.getenv('MODEL_TEMP', default = 0.0), | |
| 'tool_choice' : ('auto' if toolboxes else None), | |
| 'parallel_tool_calls' : (parallel_tool_calls if toolboxes else None)} | |
| model_params = {'temperature': os.getenv('MODEL_TEMP', default = 0.0), | |
| 'tool_choice': ('auto' if toolboxes else None), | |
| 'parallel_tool_calls': (parallel_tool_calls if toolboxes else None)} |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation logic if not set(models_params.keys()).difference(model_keys).issubset(set([])) is unnecessarily complex. The condition .issubset(set([])) is equivalent to checking if the set is empty. This should be simplified to if set(models_params.keys()) - set(model_keys): or if not set(models_params.keys()).issubset(model_keys): for better readability.
| if not set(models_params.keys()).difference(model_keys).issubset(set([])): | |
| if not set(models_params.keys()).issubset(model_keys): |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message says "Settings section...contains models that are not in the model section" but should say "models that are not in the models section" (with an 's' on 'models'). Additionally, this error message could be more helpful by indicating which models are problematic. Consider: f"Settings section of model_config file {model_config} contains models that are not defined in the models section: {', '.join(set(models_params.keys()) - set(model_keys))}"
| if not set(models_params.keys()).difference(model_keys).issubset(set([])): | |
| raise ValueError(f"Settings section of model_config file {model_config} contains models that are not in the model section") | |
| problematic_models = set(models_params.keys()) - set(model_keys) | |
| raise ValueError( | |
| f"Settings section of model_config file {model_config} contains models that are not defined in the models section: {', '.join(problematic_models)}" | |
| ) |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after comma in for k,v in models_params.items():. According to PEP 8, there should be a space after the comma: for k, v in models_params.items():.
| for k,v in models_params.items(): | |
| for k, v in models_params.items(): |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Missing space after comma in the loop. Should be for k, v in instead of for k,v in according to PEP 8 style guidelines.
| for k,v in models_params.items(): | |
| for k, v in models_params.items(): |
Check warning
Code scanning / CodeQL
Variable defined multiple times Warning
redefined
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assignment to 'name' is unnecessary as it is redefined before this value is used.
| name = task_body.get('name', 'taskflow') # placeholder, not used yet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example uses
gpt-5-miniwhich may not be a valid model identifier. As of January 2025, there is no GPT-5 model released by OpenAI. Consider using a more realistic example model name likegpt-4o-minior a placeholder likeyour-model-nameto avoid confusion.