Skip to content
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

Fixing sorting of not ordered tool calls #122

Merged
merged 1 commit into from
Nov 15, 2024
Merged

Conversation

jamesbraza
Copy link
Collaborator

#115 forgot sorting of the not ordered case

@jamesbraza jamesbraza added the bug Something isn't working label Nov 13, 2024
@jamesbraza jamesbraza self-assigned this Nov 13, 2024
@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Nov 13, 2024
Copy link
Collaborator

@sidnarayanan sidnarayanan left a comment

Choose a reason for hiding this comment

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

I'd say worth a test, but DummyEnv might be too simple to actually test this. Probably a sign that we need a more complex env for tests (came up in a recent ldp PR too).

@whitead
Copy link
Contributor

whitead commented Nov 14, 2024

Why do non-ordered tool calls need to be sorted? Isn't the point of non-ordered that they aren't in order?

@jamesbraza
Copy link
Collaborator Author

Why do non-ordered tool calls need to be sorted? Isn't the point of non-ordered that they aren't in order?

In exec_tool_calls args, ordered means non-concurrent, and not ordered means concurrent. It's a suboptimal variable name for sure, if you look at the docstring:

        ordered: Opt-in flag for forcing sequential execution (according to order
            in the above message), otherwise tool calls are made concurrently.

I think given tool_calls is a list (and not a set), we generally always care to preserve the sequence

@jamesbraza jamesbraza merged commit 9906a22 into main Nov 15, 2024
6 checks passed
@jamesbraza jamesbraza deleted the ordered-sorting branch November 15, 2024 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants