add support for model parameters passing #98

m-y-mo · 2025-11-28T09:37:08Z

No description provided.

Copilot

Pull request overview

This PR adds support for passing model-specific parameters through the task configuration system, allowing users to configure model behavior at multiple levels (global defaults, model_config file, and per-task settings).

Key Changes

Introduced a new parameter passing mechanism that allows model settings to be specified in model_config files and per-task configurations
Modified the deploy_task_agents function to accept model parameters via a new model_par parameter
Added validation logic to ensure model settings are properly structured and correspond to defined models

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 11 comments.

File	Description
src/seclab_taskflow_agent/main.py	Refactored model settings handling to support parameter passing from model_config and task definitions, added validation for model_settings structure
doc/GRAMMAR.md	Added documentation for the new `model_settings` task-level configuration option
README.md	Added documentation for the new `model_settings` section in model_config files, including validation rules and override behavior

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-28T09:41:09Z

src/seclab_taskflow_agent/__main__.py

+    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)}


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).

Suggested change

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 · 2025-11-28T09:41:10Z

src/seclab_taskflow_agent/__main__.py

-        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),


Inconsistent spacing around the = operator. Line 133 uses default = 0.0 with spaces, which deviates from PEP 8 style guidelines. It should be default=0.0 without spaces around the equals sign in function arguments.

Suggested change

model_params = {'temperature' : os.getenv('MODEL_TEMP', default = 0.0),

model_params = {'temperature' : os.getenv('MODEL_TEMP', default=0.0),

Copilot · 2025-11-28T09:41:10Z

src/seclab_taskflow_agent/__main__.py

+                raise ValueError(f"Settings section of model_config file {model_config} must be a dictionary")
+            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")
+            for k,v in models_params.items():


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():.

Suggested change

for k,v in models_params.items():

for k, v in models_params.items():

README.md

Copilot · 2025-11-28T09:41:11Z

README.md

+      effort: high
+```
+
+You do not need to set parameters for all models defined in the `models` section. When parameters is not set for a model, they'll fall back to the default value. However, all the settings in this section must belong to one of the models specified in the `models` section, otherwise an error will raise:


Grammatical error: "an error will raise" should be "an error will be raised" (passive voice) or "the system will raise an error" (active voice).

Suggested change

You do not need to set parameters for all models defined in the `models` section. When parameters is not set for a model, they'll fall back to the default value. However, all the settings in this section must belong to one of the models specified in the `models` section, otherwise an error will raise:

You do not need to set parameters for all models defined in the `models` section. When parameters is not set for a model, they'll fall back to the default value. However, all the settings in this section must belong to one of the models specified in the `models` section, otherwise an error will be raised:

Copilot · 2025-11-28T09:41:11Z

src/seclab_taskflow_agent/__main__.py

-        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),


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.

Suggested change

model_params = {'temperature' : os.getenv('MODEL_TEMP', default = 0.0),

model_params = {'temperature' : float(os.getenv('MODEL_TEMP', default='0.0')),

Copilot · 2025-11-28T09:41:12Z

src/seclab_taskflow_agent/__main__.py

+            models_params = m_config.get('model_settings', {})
+            if models_params and not isinstance(models_params, dict):
+                raise ValueError(f"Settings section of model_config file {model_config} must be a dictionary")
+            if not set(models_params.keys()).difference(model_keys).issubset(set([])):


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.

Suggested change

if not set(models_params.keys()).difference(model_keys).issubset(set([])):

if not set(models_params.keys()).issubset(model_keys):

Copilot · 2025-11-28T09:41:12Z

doc/GRAMMAR.md

+Parameters to the model can also be specified in the task using the `model_settings` section:
+
+```yaml
+    model: gpt-5-mini


The example uses gpt-5-mini which 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 like gpt-4o-mini or a placeholder like your-model-name to avoid confusion.

Suggested change

model: gpt-5-mini

model: gpt-4o-mini

src/seclab_taskflow_agent/__main__.py

Copilot · 2025-11-28T09:41:12Z

src/seclab_taskflow_agent/__main__.py

+            model_settings.update(task_model_settings)
+
            # parse our taskflow grammar
            name = task_body.get('name', 'taskflow') # placeholder, not used yet


This assignment to 'name' is unnecessary as it is redefined before this value is used.

Suggested change

name = task_body.get('name', 'taskflow') # placeholder, not used yet

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-28T09:51:59Z

src/seclab_taskflow_agent/__main__.py

+                raise ValueError(f"Settings section of model_config file {model_config} must be a dictionary")
+            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")
+            for k,v in models_params.items():


[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.

Suggested change

for k,v in models_params.items():

for k, v in models_params.items():

Copilot · 2025-11-28T09:52:00Z

src/seclab_taskflow_agent/__main__.py

+            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")


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))}"

Suggested change

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)}"

)

add support for model parameters passing

6d99fd7

Copilot AI review requested due to automatic review settings November 28, 2025 09:37

m-y-mo requested review from JarLob, Kwstubbs, kevinbackhouse, p- and sylwia-budzynska as code owners November 28, 2025 09:37

Copilot started reviewing on behalf of m-y-mo November 28, 2025 09:37 View session

m-y-mo mentioned this pull request Nov 28, 2025

Add more flexibility to configure LLM model parameters #82

Open

Copilot finished reviewing on behalf of m-y-mo November 28, 2025 09:39

Copilot AI reviewed Nov 28, 2025

View reviewed changes

Update src/seclab_taskflow_agent/__main__.py

29a9a6b

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings November 28, 2025 09:48

Update README.md

d0dff7f

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot started reviewing on behalf of m-y-mo November 28, 2025 09:48 View session

Copilot finished reviewing on behalf of m-y-mo November 28, 2025 09:50

Copilot AI reviewed Nov 28, 2025

View reviewed changes

kevinbackhouse approved these changes Nov 28, 2025

View reviewed changes

m-y-mo merged commit 4377732 into main Nov 28, 2025
9 checks passed

m-y-mo deleted the model_settings branch November 28, 2025 13:17

	model_params = {'temperature' : os.getenv('MODEL_TEMP', default = 0.0),
	model_params = {'temperature' : os.getenv('MODEL_TEMP', default=0.0),

	for k,v in models_params.items():
	for k, v in models_params.items():

	You do not need to set parameters for all models defined in the `models` section. When parameters is not set for a model, they'll fall back to the default value. However, all the settings in this section must belong to one of the models specified in the `models` section, otherwise an error will raise:
	You do not need to set parameters for all models defined in the `models` section. When parameters is not set for a model, they'll fall back to the default value. However, all the settings in this section must belong to one of the models specified in the `models` section, otherwise an error will be raised:

	model_params = {'temperature' : os.getenv('MODEL_TEMP', default = 0.0),
	model_params = {'temperature' : float(os.getenv('MODEL_TEMP', default='0.0')),

	if not set(models_params.keys()).difference(model_keys).issubset(set([])):
	if not set(models_params.keys()).issubset(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")

add support for model parameters passing #98

add support for model parameters passing #98

Uh oh!

Conversation

m-y-mo commented Nov 28, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants