Skip to content

Conversation

denakorita
Copy link
Contributor

Problem

Taxonomy search should be optimised to reduce the time spent exploring entities and properties when building HogQL

Copy link
Contributor

github-actions bot commented Sep 29, 2025

Size Change: 0 B

Total Size: 3.05 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 3.05 MB

compressed-size-action

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

11 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@denakorita denakorita requested a review from a team October 6, 2025 15:18
Copy link
Contributor

@daibhin daibhin left a comment

Choose a reason for hiding this comment

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

Error tracking pieces looks fine. Tried reviewing the rest but honestly it's probably worth someone with an understanding of Max's internals having a look

Comment on lines 204 to 206
# If we're still here, check if we've hit the iteration limit within this cycle
if state.iteration_count >= self.MAX_ITERATIONS:
return self._get_reset_state(ITERATION_LIMIT_PROMPT, "max_iterations", state)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it check for LLM turns, not for the number of tool calls? It doesn't make sense to limit by tool count.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The state.iteration_count increases by one only when the llm returns, I think I am making a mistake here by checking for its value in every tool_call processing, we should only check once. Let me know if that makes sense.

for action, _ in intermediate_steps:
try:
tool_input = self._toolkit.get_tool_input_model(action)
except ValidationError as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

One might be broken, but others might not be. We should process all generated tools.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, fixed that

tool_call_id=action.log,
)
tool_result_msg.append(tool_msg)
tool_msgs.append(tool_msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move tool_msgs closer to the loop? They're quite distant now.


messages: Sequence[AssistantMessageUnion] = Field(default=[])

iteration_count: int = Field(default=0)
Copy link
Contributor

Choose a reason for hiding this comment

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

There may be some quirks with LangGraph defaults. Are you sure that the Tools node doesn't reset the iteration count when it returns the partial state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, the tool node resets the count, I had to pass the iteration_count when returning the partial state there

if tool_metadata["lookup_feature_flag"]:
tool_input, tool_call_id = tool_metadata["lookup_feature_flag"][0]
result = self._lookup_feature_flag(tool_input.arguments.flag_key) # type: ignore
return {tool_call_id: result}
Copy link
Contributor

Choose a reason for hiding this comment

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

This line will only handle a single tool, but it might have multiple tools.

@denakorita denakorita requested a review from a team as a code owner October 14, 2025 09:53
@denakorita denakorita removed the request for review from a team October 14, 2025 10:03
@denakorita denakorita force-pushed the feat/paralel-tool-calls branch from 8a3b67b to 144c1d4 Compare October 14, 2025 10:14
@PostHog PostHog deleted a comment from github-actions bot Oct 14, 2025
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 12)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

@skoob13 skoob13 left a comment

Choose a reason for hiding this comment

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

LGTM

@denakorita denakorita merged commit c96563d into feat/allow-multiple-entity-event-search Oct 16, 2025
180 of 181 checks passed
@denakorita denakorita deleted the feat/paralel-tool-calls branch October 16, 2025 11:46
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