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

[FEATURE] Order of tools in Register Agent Step should be deterministic #281

Closed
dbwiddis opened this issue Dec 12, 2023 · 4 comments · Fixed by #289
Closed

[FEATURE] Order of tools in Register Agent Step should be deterministic #281

dbwiddis opened this issue Dec 12, 2023 · 4 comments · Fixed by #289
Assignees
Labels
enhancement New feature or request untriaged

Comments

@dbwiddis
Copy link
Member

Is your feature request related to a problem?

For some agents (Flow Agent), the order of the tools matters. Currently when creating the list of tools we are using non-deterministic ordering from a HashMap entry set to create the previousNodes list.

List<String> previousNodes = previousNodeInputs.entrySet()
    .stream()
    .filter(e -> TOOLS_FIELD.equals(e.getValue()))
    .map(Map.Entry::getKey)
    .collect(Collectors.toList());

We need a way to (optionally) enforce an ordering of these tools.

What solution would you like?

The AgentStep already has an optional "tools" field in the code but it is never read, instead just iterating over the previous node inputs and looking for "tool" in the value.

If that field is populated with an array of tool ID's (the workflow step ID they were created with), the list should be sorted in that order.

What alternatives have you considered?

Always sort in order of workflow step ID. However, we may want different agents with different orderings created from the same definition.

@dbwiddis dbwiddis added enhancement New feature or request untriaged labels Dec 12, 2023
@dbwiddis dbwiddis self-assigned this Dec 12, 2023
@dbwiddis
Copy link
Member Author

dbwiddis commented Dec 13, 2023

We need to decide how to handle (or whether to require) duplicate input.

Current setup:

"previous_node_inputs": {
    "some_step": "some_value",
    "foo_tool": "tools",
    "bar_tool": "tools",
    "baz_tool": "tools"
},

Result: foo, bar and baz tools in any order.

Possible setup A:

"previous_node_inputs": {
    "some_step": "some_value",
    "foo_tool": "tools",
    "bar_tool": "tools",
    "baz_tool": "tools"
},
"tools": ["foo_tool", "bar_tool", "baz_tool"],

Result: foo, bar and baz tools in that specific order.

But do we need the redundancy?

Possible setup B:

"previous_node_inputs": {
    "some_step": "some_value"
},
"tools": ["foo_tool", "bar_tool", "baz_tool"],

Result: foo, bar and baz tools in that specific order. Input from previous node assumed.

But what if someone does this:

Possible setup C:

"previous_node_inputs": {
    "some_step": "some_value",
    "foo_tool": "tools",
    "bar_tool": "tools",
    "baz_tool": "tools"
},
"tools": ["foo_tool"],

Result option 1: foo tool only.
Result option 2: foo tool first, followed by bar and baz in any order.

Possible setup D:

"previous_node_inputs": {
    "some_step": "some_value",
    "bar_tool": "tools",
    "baz_tool": "tools"
},
"tools": ["foo_tool", "bar_tool"],

Result option 1: foo tool and bar tool in that order.
Result option 2: foo tool and bar tool first, in that order followed by baz in any order (which is just baz)
Result option 3: fail validation

Ultimately we need this to be simple and easy to explain. So

Option 1 (my preference, works with A, B, C2, and D2):

  • tools can be specified as previous node inputs OR on a tools field with those inputs assumed
  • all listed tools in either location will be used, with duplicates ignored; those (if any) in tools field will be ordered first

Option 2 (works with A, B, C1, and D1):

  • tools must be specified in only one location, either previous node inputs or tools field. If tools field exists, entries in previous inputs will be ignored, and they will be ordered.

Option 3 (works with A, C1, and D3):

  • tools must always be specified in previous node inputs
  • if present, tools field will be the only tools used, and will fail validation if not included in inputs

@dbwiddis
Copy link
Member Author

Another idea:

"previous_node_inputs": {
    "some_step": "some_value",
    "foo_tool": "tools.1",
    "bar_tool": "tools.2",
    "baz_tool": "tools.3"
}

@dbwiddis
Copy link
Member Author

OK, currently going with "Option 1":

  • Anything in "tools" field is placed first in order, whether or not it exists in previous node inputs
  • Anything else in previous node inputs is added in nonspecific order
// Anything in tools is sorted first, followed by anything else in previous node inputs
List<String> sortedNodes = tools == null ? new ArrayList<>() : Arrays.asList(tools);
previousNodes.removeAll(sortedNodes);
sortedNodes.addAll(previousNodes);
sortedNodes.forEach((node) -> {

@dbwiddis
Copy link
Member Author

Reasoning for "option 1":

  • Validation (and front-end tie-in) will be conducted with the "previous node inputs" so it's good to have them
  • Ordering with the "nodes" field is an optional thing and it's good to allow it when provided but I'm really hesitant to throw a runtime validation failure if it doesn't list all the things.
  • Doesn't "silently fail" on expected results or throw runtime exceptions following pre-run validation
  • Can be easily explained in docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request untriaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant