Adds action tagging

[elijahbenizzy]

This enables us to use tags as aliases for actions. We may add future tags (E.G. __requires_inputs), but for now this is all done on behalf of the user. The idea is to have a very simple polymorphic interface -- we can (for example) say all nodes tagged with "text_return" fullfill the same role (displaying markdown for the user).

See #468 for more details.

[Short description explaining the high-level reason for the pull request]

Changes

• Implements tagging interface
• Adds unit/integration tests
• Docs
  • ensure docstrings
  • concepts/whatnot
• E2E test (at least one)

How I tested this

Tests + manual.

Notes

Checklist

• PR has an informative and human-readable title (this will be pulled into the release notes)
• Changes are limited to a single goal (no scope creep)
• Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
• Any change in functionality is tested
• New functions are documented (with a description, list of inputs, and expected output)
• Placeholder code is flagged / future TODOs are captured in comments
• Project documentation has been updated if adding/changing functionality.

---

Important

Introduces action tagging to group actions by aliases, updating core classes, tests, and documentation.

• Behavior:
  • Introduces action tagging, allowing actions to be tagged with aliases for grouping and identification.
  • Updates Application class to process control flow parameters with tags in _process_control_flow_params().
  • Supports tags in halt_before and halt_after parameters in Application methods.
• Graph:
  • Adds _create_action_tag_map() to map tags to actions in Graph.
  • Implements get_actions_by_tag() to retrieve actions by tag.
• Pydantic Integration:
  • Adds tags parameter to pydantic_action() and pydantic_streaming_action().
• Tests:
  • Adds tests for action tagging in test_action.py, test_application.py, and test_graph.py.
• Documentation:
  • Updates actions.rst to include information on action tagging.

^{This description was created by for 4123a9e. It will automatically update as commits are pushed.}

[github-actions]

A preview of 4123a9e is uploaded and can be seen here:

✨ https://burr.dagworks.io/pull/505 ✨

Changes may take a few minutes to propagate. Since this is a preview of production, content with draft: true will not be rendered. The source is here: https://github.com/DAGWorks-Inc/burr/tree/gh-pages/pull/505/

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to 142ef4a in 1 minute and 13 seconds

More details

• Looked at 688 lines of code in 8 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. burr/core/application.py:1076

• Draft comment:
  The method _clean_iterate_params has been renamed to _process_control_flow_params and now includes logic to handle tags in halt conditions. Ensure that all references to this method are updated accordingly.
• Reason this comment was not posted:
  Comment did not seem useful.

Workflow ID: wflow_HPUev4OWFT7vyJnI

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[skrawcz]

can this be applied to edges too?

[skrawcz]

on thinking more applying to edges is fraught - so should mention where this alias is to be used?

[skrawcz]

Can we get the UX of how someone uses this scoped first?

I'm of the strong opinion we should demarcate what is a tag and what isn't in the control flow. E.g.

 ...  = app.run(halt_before=["@tag:display_inputs"], inputs=...)

With a straight up alias I see these issues otherwise:

1. What if an action has the same name?
2. When reading the control flow you don't know what it refers to easily without knowing the actions themselves.

So requirements thoughts:

1. When someone reads the control flow it should be clear that tags are special. Since tags could map to multiple nodes. Hence why I like a prefix.
2. You should be able to have an action name and tag be the same thing, you'd distinguish them with one being @tag: or something.
3. From an edge perspective, you could see tags functioning on the "source" side of the edge to expand that and route to a single node, e.g. error node.
4. You should be able to get the mapping of tags -> actions from the graph/application object.
5. I don't see an error case, but with the above an action can have multiple tags and things should all just work.
6. We might want a tag object, that way people don't need to update strings, and instead can use the pointer to that object... (though we need this more broadly to be able to replace "strings" when defining a graph/application).

[elijahbenizzy]

Can we get the UX of how someone uses this scoped first?

I'm of the strong opinion we should demarcate what is a tag and what isn't in the control flow. E.g.

 ...  = app.run(halt_before=["@tag:display_inputs"], inputs=...)

With a straight up alias I see these issues otherwise:

1. What if an action has the same name?
2. When reading the control flow you don't know what it refers to easily without knowing the actions themselves.

So requirements thoughts:

1. When someone reads the control flow it should be clear that tags are special. Since tags could map to multiple nodes. Hence why I like a prefix.
2. You should be able to have an action name and tag be the same thing, you'd distinguish them with one being @tag: or something.
3. From an edge perspective, you could see tags functioning on the "source" side of the edge to expand that and route to a single node, e.g. error node.
4. You should be able to get the mapping of tags -> actions from the graph/application object.
5. I don't see an error case, but with the above an action can have multiple tags and things should all just work.
6. We might want a tag object, that way people don't need to update strings, and instead can use the pointer to that object... (though we need this more broadly to be able to replace "strings" when defining a graph/application).

Yeah, thought through it a bit. Trade-offs:

1. On one hand you've got a bespoke UX -- @tag: -- is this a common pattern? Are there others (@requires_input:? @streaming_response:)? feels like it'll likely be a one-off, althought requires_input is OK. It's also string magic
2. On the other hand it's readable -- clear what's happening.

My thought was to make them indistinguishable from each other -- if they have the right tag then they effectively function the same. I'm not sure the value in distinguishing them -- if we have it so names are true aliases, then it's very clear that two nodes are interchangeable.

That said @tag does improve clarity, but I wanted to keep the API simple. Regarding an action with the same name -- that's a user error, but there's an interesting point -- tags are set at the action-level whereas names are set at the application level (often with defaults at the action-level)... One could also add tags at the application level as well .with_tags(...), which we have not implemented yet.

So yeah, I'm not against "@tag:<tag_name>" even though it's quite a bit uglier -- I think the clarity might be worthwhile worthwhile.

[skrawcz]

1. On one hand you've got a bespoke UX -- @tag: -- is this a common pattern? Are there others (@requires_input:? @streaming_response:)? feels like it'll likely be a one-off, althought requires_input is OK. It's also string magic

Those are properties of the action though? They could be property. E.g. @property:requires_input @property:streaming_response.

My thought was to make them indistinguishable from each other -- if they have the right tag then they effectively function the same. I'm not sure the value in distinguishing them -- if we have it so names are true aliases, then it's very clear that two nodes are interchangeable.

I don't think that's the problem being solved here. To me it's:

1. Not having to manually list all actions.
2. Being able to divorce function/class name from that of provided action name to the application. I.e. one less thing to update if you change the names of things. Tags would be invariant here. (though likely need a way to remove tags for edge cases here).

We err on the side of making things more readable, and to try to help a user mentally map what is going on / is different from just an "action name". E.g. @tag:... could correspond to 1 or more actions, which isn't true the other way around. So I think it would be something hard to walk back if we allowed it to look like a regular action.

[elijahbenizzy]

1. On one hand you've got a bespoke UX -- @tag: -- is this a common pattern? Are there others (@requires_input:? @streaming_response:)? feels like it'll likely be a one-off, althought requires_input is OK. It's also string magic

Those are properties of the action though? They could be property. E.g. @property:requires_input @property:streaming_response.

My thought was to make them indistinguishable from each other -- if they have the right tag then they effectively function the same. I'm not sure the value in distinguishing them -- if we have it so names are true aliases, then it's very clear that two nodes are interchangeable.

I don't think that's the problem being solved here. To me it's:

1. Not having to manually list all actions.
2. Being able to divorce function/class name from that of provided action name to the application. I.e. one less thing to update if you change the names of things. Tags would be invariant here. (though likely need a way to remove tags for edge cases here).

We err on the side of making things more readable, and to try to help a user mentally map what is going on / is different from just an "action name". E.g. @tag:... could correspond to 1 or more actions, which isn't true the other way around. So I think it would be something hard to walk back if we allowed it to look like a regular action.

So (1) is the goal here -- (2) I'd rephrase as "being able to divorce the actual instance of the action from the stated goal (E.G. polymorphism), allowing easier iteration. We don't use the class name, and the function name is just a convenience.

So I'm ok with @tag:... for readability, but also want to add .with_tags(...) I think.

[ellipsis-dev]

❌ Changes requested. Incremental review on 7b00abd in 1 minute and 21 seconds

More details

• Looked at 719 lines of code in 8 files
• Skipped 0 files when reviewing.
• Skipped posting 9 drafted comments based on config settings.

1. tests/core/test_graph.py:10

• Draft comment:
  Good use of a dedicated PassedInAction class to simulate actions for testing the graph. Consider adding type hints to the class methods for consistency with the rest of the codebase.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

2. tests/core/test_graph.py:127

• Draft comment:
  Tests for 'get_actions_by_tag' correctly verify that filtering by tags works. Consider adding a test case for an unknown tag to verify that a ValueError is raised.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

3. tests/core/test_graph.py:101

• Draft comment:
  Test 'test_graph_builder_builds' confirms that the builder creates the expected number of actions and transitions. Ensure that when multiple with_transitions calls are made, the builder combines them as expected.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

4. tests/core/test_graph.py:113

• Draft comment:
  Test 'test_graph_builder_get_next_node' verifies the basic next node selection. It may be worth adding tests with non-default conditions where multiple transitions exist.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

5. burr/integrations/pydantic.py:170

• Draft comment:
  The pydantic_action and pydantic_streaming_action decorators now accept a 'tags' parameter which is forwarded to FunctionBasedAction. Confirm that the new tags parameter is correctly documented in the docstrings for these functions.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

6. docs/concepts/actions.rst:292

• Draft comment:
  The new documentation for action tagging is clear. Consider adding a note about the expected syntax (e.g., '@tag:<tag_name>') and the behavior when a tag does not exist, in light of the behavior in get_actions_by_tag.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

7. tests/core/test_action.py:235

• Draft comment:
  Tests for function‐based actions verify that tags are correctly copied and accessible. The tests are comprehensive; no issues noted.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

8. tests/core/test_application.py:3535

• Draft comment:
  Good test coverage for control flow parameters with tags. All expected behavior is verified. Consider also testing behavior when a non‐existent tag is passed in to the control flow parameters.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

9. tests/core/test_graph.py:127

• Draft comment:
  Tests for graph tag mapping (get_actions_by_tag) appear to cover basic functionality. Ensure that behavior (error vs empty list) aligns with design decisions.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

Workflow ID: wflow_0IZKiswyQGyqdqfX

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on d5fd718 in 1 minute and 49 seconds

More details

• Looked at 721 lines of code in 8 files
• Skipped 0 files when reviewing.
• Skipped posting 12 drafted comments based on config settings.

1. tests/core/test_graph.py:10

• Draft comment:
  Nice implementation of PassedInAction with tag support. Consider documenting in the class docstring that if no tags are provided, an empty list is returned.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. tests/core/test_graph.py:127

• Draft comment:
  The test for validating transitions with tags looks good. Consider adding further tests to verify behavior when multiple actions share the same tag and when no actions have a given tag (return empty list rather than raising an error) if such behavior is desired.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The comment suggests testing cases that are actually already tested: multiple actions sharing tags (tested with tag1) and no actions having a tag (tested with tag4). The comment seems to have missed that these cases are already covered in the existing test. The suggestion about returning an empty list instead of raising an error appears to be a feature request rather than a test suggestion.
  Maybe there are other edge cases that could be tested, like empty tag strings or None tags? The comment could be pointing to valid test gaps I haven't considered.
  The existing test already covers the core functionality comprehensively. Any additional edge cases would be implementation details that should be driven by actual requirements rather than test suggestions.
  The comment should be deleted as it suggests adding test cases that are already present in the code and makes implementation suggestions that go beyond testing the current functionality.

3. docs/concepts/actions.rst:10

• Draft comment:
  Typo: Use 'asynchronous' instead of 'asynchonous' for clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. tests/core/test_action.py:238

• Draft comment:
  Excellent coverage: the test for function‐based actions with inputs covers both required and default values.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

5. tests/core/test_application.py:3535

• Draft comment:
  Good integration testing of control flow with tags. Consider also adding tests for actions with no tags, ensuring default empty list behavior.
• Reason this comment was not posted:
  Marked as duplicate.

6. tests/core/test_graph.py:127

• Draft comment:
  The test for get_actions_by_tag demonstrates correct behavior; consider adding a case where an action has no tags to assert it returns an empty list.
• Reason this comment was not posted:
  Marked as duplicate.

7. burr/integrations/pydantic.py:175

• Draft comment:
  Nice integration of the new 'tags' parameter in the pydantic action decorator. Ensure that type annotations clearly indicate tags are Optional[List[str]].
• Reason this comment was not posted:
  Comment looked like it was already resolved.

8. tests/core/test_application.py:3070

• Draft comment:
  Very thorough tests for Application context propagation via __context. Consider adding an explicit test for an action that does not declare tags to confirm default behavior.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

9. tests/core/test_application.py:3190

• Draft comment:
  The tests for remapping dunder parameters (context and tracer) are comprehensive. The use of f-strings with mangled names is clear.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

10. tests/core/test_application.py:3535

• Draft comment:
  Use of 'collections.deque(generator, maxlen=0)' to fully exhaust generators is clever and efficient for testing streaming actions.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

11. tests/core/test_graph.py:122

• Draft comment:
  GraphBuilder tests are well structured, covering valid and error cases for transitions and actions. Well done.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

12. tests/core/test_application.py:2668

• Draft comment:
  Tests simulate recursive application behavior with lifecycle hooks effectively; this adds valuable regression coverage for complex recursive workflows.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

Workflow ID: wflow_WO3fW5Pfr7FAj6Vd

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[elijahbenizzy]

@skrawcz and I talked offline -- I think @tag:... is probably better, added that. Will test then merge tomorrow.

[skrawcz]

?

[ellipsis-dev]

👍 Looks good to me! Incremental review on 4123a9e in 1 minute and 37 seconds

More details

• Looked at 721 lines of code in 8 files
• Skipped 0 files when reviewing.
• Skipped posting 13 drafted comments based on config settings.

1. tests/core/test_graph.py:18

• Draft comment:
  The PassedInAction class implementation properly returns tags (with a fallback to an empty list), which is good. No issues here, but consider adding a brief docstring for clarity on the tags parameter for future maintainers.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

2. tests/core/test_graph.py:127

• Draft comment:
  The test for get_actions_by_tag is clear and asserts the expected counts. Consider adding comments explaining why tag 'tag1' should return 2 elements, to aid in readability of tests.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

3. tests/core/test_application.py:64

• Draft comment:
  The async tests rely on direct 'await' calls and async-for loops. For consistency and clearer test organization, consider using the pytest.mark.asyncio decorator on async test functions.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

4. tests/core/test_application.py:1418

• Draft comment:
  The test checking that inputs with a leading double underscore are rejected (test_application_does_not_allow_dunderscore_inputs) works fine. Consider adding a comment explaining that this restriction is by design to preserve internal keys.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

5. tests/core/test_application.py:3270

• Draft comment:
  The test using the ApplicationBuilder to set a spawning parent is clear. In test_application_with_spawning_parent, consider clarifying via inline comments the significance of each assertion for app.spawning_parent_pointer.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

6. tests/core/test_application.py:3430

• Draft comment:
  Tests for remapping dunder variables in actions (e.g. __context, __tracer) are correct. Consider adding a comment on the expected mangling scheme for clarity (i.e. how the name is prefixed with _).
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

7. tests/core/test_application.py:3535

• Draft comment:
  The test_application__process_control_flow_params test is comprehensive; consider adding a sentence in the docstring explaining the expected expansion behavior from tags to action names.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

8. tests/core/test_graph.py:290

• Draft comment:
  The GraphBuilder.with_transitions API is used in tests. The use of tuples for transitions is clear, but consider adding a short inline comment in the test to describe the rationale behind each transition, even if it duplicates information from the docstring.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

9. tests/core/test_graph.py:10

• Draft comment:
  Consider abstracting the PassedInAction test stub (lines 10-48) into a shared test utility to avoid duplication with similar stubs in other test files. Maintaining a common test helper improves consistency and reduces maintenance overhead.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

10. tests/core/test_graph.py:69

• Draft comment:
  The test for redundant transitions (lines 69-76) is clear. As an enhancement, consider verifying that the error message includes details about the offending transition (e.g. the source action name) to aid debugging.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

11. tests/core/test_graph.py:127

• Draft comment:
  The test for get_actions_by_tag (lines 127-157) is comprehensive. Additionally, consider adding a test case where an action has an empty tag list to verify that tag lookup handles empty tags correctly.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

12. tests/core/test_graph.py:100

• Draft comment:
  GraphBuilder tests cover key functionality. For future improvement, consider adding tests for graph visualization (e.g. validating that a non-null graphviz object is produced).
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

13. tests/core/test_graph.py:140

• Draft comment:
  Consider adding inline comments or brief function docstrings for complex test cases (lines 140-158) to help future maintainers quickly grasp the purpose and expected behavior of these tests.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

Workflow ID: wflow_hetJqWiEETa5oUyq

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.