Skip to content

Conversation

@skoob13
Copy link
Contributor

@skoob13 skoob13 commented Oct 17, 2025

Problem

Since we want to parallelize tool calls, we would like to split the monolithic graph into subgraphs or MaxTools, allowing the tools to be easily parallelized. This PR focuses on the insights subgraph.

Changes

  • Avoid circular import exceptions by splitting the BaseAssistantGraph to a separate module.
  • Implement a unified MaxTool that handles both creating and editing an insight.
  • Add tests.

How did you test this code?

Unit tests & manual testing

@skoob13 skoob13 force-pushed the feat/split-insight-tool branch from 027ed1a to 0f3499f Compare October 20, 2025 11:08
@skoob13 skoob13 marked this pull request as ready for review October 20, 2025 17:28
@skoob13 skoob13 requested a review from a team October 20, 2025 17:28
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Oct 20, 2025

Greptile encountered an error while reviewing this PR. Please reach out to [email protected] for assistance.


def add_node(self, node: MaxNodeName, action: Any):
self._graph.add_node(node, action)
if isinstance(action, HasReasoningMessage):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remembered that isinstance on Protocol(s) with runtime_checkable was pretty slow but maybe this changed with recent python releases, cannot find anything recent, just wanted to flag if we could've have done this on some magic variables instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know about that. ChatGPT confirms it, but it's still nanoseconds, so it's okay for a single check. I don't see a reason not to use it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a great memory so maybe ChatGPT is better lol, but I think it was related to accessing @property so maybe it's okay

Copy link
Contributor

@kappa90 kappa90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:yolo:


from ee.hogai.graph.base import BaseAssistantNode
from ee.hogai.graph.graph import AssistantCompiledStateGraph
from ee.hogai.graph.base import AssistantCompiledStateGraph, BaseAssistantNode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, AssistantCompiledStateGraph is gone on my loop-executor-ui-full branch, so there will be conflicts (I can fix them once we get there)

]

# The contextual insights tool overrides the static tool. Only inject if it's injected.
if not CreateAndQueryInsightTool.is_editing_mode(self.context_manager):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI on my branch I've created two distinct tools, one to create and one to edit, so that we can have distinct definitions, but we should probably discuss this better, I don't have a strong opinion.

@skoob13 skoob13 merged commit 6403cd2 into feat/parallelize-tool-calls Oct 22, 2025
100 of 164 checks passed
@skoob13 skoob13 deleted the feat/split-insight-tool branch October 22, 2025 11:56
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.

4 participants