fix: change tool_calls from dict to list by akihikokuroda · Pull Request #1435 · generative-computing/mellea

akihikokuroda · 2026-07-23T22:23:50Z

Pull Request

Issue

Description

This changed tool_calls from dict to list instead of set because of the follow reason

Aspect	Dict	Set	List
Parallel same-name calls	❌ Collapses	✓ Preserved	✓ Preserved
Execution order	✓ Preserved	❌ Lost	✓ Preserved
Duplicate handling	Overwrites silently	Deduplicates silently	Must validate explicitly
Component prefixing	✓ Works	❌ Order lost	✓ Works
Requires `__hash__`/`__eq__`	No	Yes	No
Equality semantics	N/A	Unclear	N/A
Observability	✓ Simple	❌ Hidden deduplication	✓ Visible duplicates
Implementation effort	N/A	Medium (add hash/eq)	Low (validate helper)

Testing

Tests added to the respective file if code was changed
New code has 100% coverage if code was added
Ensure existing tests and github automation passes (a maintainer will kick off the github automation when the rest of the PR is populated)

Attribution

AI coding assistants used

Adding a new component, requirement, sampling strategy, or tool?

If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.

Component
Requirement
Sampling Strategy
Tool

NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.

akihikokuroda · 2026-07-24T14:26:25Z

This has broader changes than expected.

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

jakelorocco

Thank you! Looks mostly good. A few minor things I noticed.

Can you also search through our documentation for these uses? I believe examples like this will need to be modified as well:

response = m.instruct(
    "Use the code interpreter tool to compute 7 factorial.",
    requirements=[uses_tool("python")],
    model_options={ModelOption.TOOLS: [tool]},
    tool_calls=True,
)

# tool_calls=True makes .tool_calls available on the result
code = response.tool_calls["python"].args["code"]
exec_result = response.tool_calls["python"].call_func()
print(exec_result)

jakelorocco · 2026-07-29T17:40:51Z

+        assert isinstance(model_tool_calls, list), (
+            f"Expected list[ModelToolCall], got {type(model_tool_calls)}"
+        )


Same as prior; must already be a list here.

Removing assertion

jakelorocco · 2026-07-29T17:42:35Z

        tool_chunk = extract_model_tool_requests(tools, choice_response)
        if tool_chunk is not None:
-            if mot.tool_calls is None:
-                mot.tool_calls = {}
-            # Merge the tool_chunk dict.
-            for key, val in tool_chunk.items():
-                mot.tool_calls[key] = val
+            if not isinstance(tool_chunk, list):
+                MelleaLogger.get_logger().error(
+                    f"extract_model_tool_requests returned {type(tool_chunk).__name__} "
+                    f"instead of list[ModelToolCall]"
+                )


If this isn't None, do we need to check that it's a list? Doesn't our type system / the extract_model_tool_requests function ensure it's a list?

Yes, it is. Removing check.

jakelorocco · 2026-07-29T17:43:34Z

+            assert isinstance(model_tool_calls, list), (
+                f"Expected list[ModelToolCall], got {type(model_tool_calls)}"
+            )


I don't think it's possible for this to not be a list here.

Removing assertion

jakelorocco · 2026-07-29T17:46:42Z

@@ -806,38 +811,50 @@ def _extract_model_tool_requests(
        self,
        tools: dict[str, AbstractMelleaTool],
        chat_response: litellm.ModelResponse,  # type: ignore
-    ) -> dict[str, ModelToolCall] | None:
-        model_tool_calls: dict[str, ModelToolCall] = {}
+    ) -> list[ModelToolCall] | None:
+        model_tool_calls: list[ModelToolCall] = []


Are we actually using this function anywhere? It looks like this got missed in a refactor.

No, it is not. Removing it.

jakelorocco · 2026-07-29T17:48:47Z

+            assert isinstance(model_tool_calls, list), (
+                f"Expected list[ModelToolCall], got {type(model_tool_calls)}"
+            )


Same as above. I think this must be a list already.

Removing check.

jakelorocco · 2026-07-29T17:49:16Z

+            if not isinstance(tool_chunk, list):
+                MelleaLogger.get_logger().error(
+                    f"extract_model_tool_requests returned {type(tool_chunk).__name__} "
+                    f"instead of list[ModelToolCall]"
+                )


Same as above.

Removing check.

jakelorocco · 2026-07-29T17:49:51Z

+            are silently overwritten). For duplicate tool names, use component ID-based prefixing to ensure
+            unique names across components.


Can we leave out the component ID based prefixing for now?

Agreed here, I don't think we have component id prefixing?

jakelorocco · 2026-07-29T17:51:26Z

+            if not isinstance(tool_chunk, list):
+                MelleaLogger.get_logger().error(
+                    f"extract_model_tool_requests returned {type(tool_chunk).__name__} "
+                    f"instead of list[ModelToolCall]"
+                )


Same as above.

Removing check.

jakelorocco · 2026-07-29T17:51:57Z

    def _extract_model_tool_requests(
        self, tools: dict[str, AbstractMelleaTool], chat_response: dict
-    ) -> dict[str, ModelToolCall] | None:
-        model_tool_calls: dict[str, ModelToolCall] = {}
+    ) -> list[ModelToolCall] | None:
+        model_tool_calls: list[ModelToolCall] = []


I also think this function isn't getting used anywhere?

Correct. Removing.

jakelorocco · 2026-07-29T17:52:29Z

+        assert isinstance(model_tool_calls, list), (
+            f"Expected list[ModelToolCall], got {type(model_tool_calls)}"
+        )


Same as previous comments.

Removing assertion

ajbozarth

Some feedback from Claude — core dict→list conversion looks correct and complete. Agree with the cleanup items already in the existing review (redundant asserts, unreachable list-guards); not re-posting to avoid duplicate threads. Two test notes to add:

No execution-level test for the fix. The added test proves two same-name calls survive extraction, but nothing runs them through _acall_tools to confirm both execute and produce two ToolMessages — the behavior #1431 is about.
A round-trip test worth front-loading. Result-turn correlation is #1389's territory and correctly untouched here, but a test now exercising the full tool→model→toolcall→model loop for parallel same-name calls (asserting today's behavior) gives #1389 a regression guard already in place.

AngeloDanducci

I think Jake's comments covered everything I found, overall looks good pending the feedback cycle.

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

akihikokuroda · 2026-07-29T20:36:02Z

@ajbozarth a test for "No execution-level test for the fix" is added.

ajbozarth

Re-review from Claude — thanks for the updates. The cleanup items and the execution-level test are all addressed. One marker fix inline; the fix itself looks good.

ajbozarth · 2026-07-29T22:07:04Z

+from mellea.core.base import ModelOutputThunk, ModelToolCall
+from mellea.stdlib.functional import _acall_tools
+
+pytestmark = [pytest.mark.ollama, pytest.mark.e2e]


These are integration tests, not e2e/ollama. _acall_tools only touches the backend for formatter.print(), never for inference — they construct ModelToolCalls directly with local Python functions, so no model runs (they pass in ~0.2s with no Ollama server). Per test/README.md, wiring multiple real project components together without external I/O is integration (no backend marker, and not skipped when Ollama is absent) — right now the ollama marker skips this regression guard in every lane without a running Ollama. Suggest pytestmark = [pytest.mark.integration]. The backend fixture (L26) is only used for .formatter.print(), so a real OllamaModelBackend isn't required either.

Made changes.

ajbozarth · 2026-07-29T22:07:04Z

+automatic tool execution loop through _acall_tools() correctly processes
+multiple same-name tool calls from the list-based tool_calls structure.
+
+Unlike test_parallel_same_name_tool_execution.py which manually calls


This references test_parallel_same_name_tool_execution.py, which doesn't exist in the tree — dangling reference. Worth removing or pointing at the actual extraction test.

Made changes, too. Thanks!

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

akihikokuroda changed the title ~~change tool_calls from dict to list~~ fix: change tool_calls from dict to list Jul 23, 2026

github-actions Bot added the bug Something isn't working label Jul 23, 2026

akihikokuroda added 6 commits July 28, 2026 19:40

change tool_calls from dict to list

d5c1fb9

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

fix merge errors

e2e1767

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

fix merge errors

e0fbf99

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

fix merge errors

2df7343

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

fix merge errors

a6ba93a

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

review comments

2e24097

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

akihikokuroda force-pushed the issue1431 branch from 8969866 to 2e24097 Compare July 28, 2026 23:41

akihikokuroda marked this pull request as ready for review July 29, 2026 14:27

akihikokuroda requested a review from a team as a code owner July 29, 2026 14:27

akihikokuroda requested review from AngeloDanducci, ajbozarth and jakelorocco July 29, 2026 14:27

jakelorocco reviewed Jul 29, 2026

View reviewed changes

ajbozarth reviewed Jul 29, 2026

View reviewed changes

AngeloDanducci reviewed Jul 29, 2026

View reviewed changes

akihikokuroda added 2 commits July 29, 2026 15:44

review comments

e70366e

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

a test added

fa8d8a6

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

akihikokuroda requested review from AngeloDanducci, ajbozarth and jakelorocco July 29, 2026 20:36

ajbozarth reviewed Jul 29, 2026

View reviewed changes

review comments

2149fc0

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

akihikokuroda requested a review from ajbozarth July 29, 2026 23:40

		are silently overwritten). For duplicate tool names, use component ID-based prefixing to ensure
		unique names across components.

Uh oh!

Conversation

akihikokuroda commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request

Issue

Description

Testing

Attribution

Adding a new component, requirement, sampling strategy, or tool?

Uh oh!

akihikokuroda commented Jul 24, 2026

Uh oh!

jakelorocco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajbozarth left a comment

Choose a reason for hiding this comment

Uh oh!

AngeloDanducci left a comment

Choose a reason for hiding this comment

Uh oh!

akihikokuroda commented Jul 29, 2026

Uh oh!

ajbozarth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

akihikokuroda commented Jul 23, 2026 •

edited

Loading