Merged
Conversation
|
A preview of ce3d801 is uploaded and can be seen here: ✨ https://burr.dagworks.io/pull/527 ✨ Changes may take a few minutes to propagate. Since this is a preview of production, content with |
elijahbenizzy
force-pushed
the
merge-graphs-application-builder
branch
from
8a89a68 to
11eb1f1
Compare
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 11eb1f1 in 1 minute and 47 seconds
More details
- Looked at
214lines of code in4files - Skipped
0files when reviewing. - Skipped posting
11drafted comments based on config settings.
1. burr/core/graph.py:31
- Draft comment:
Consider adding a space or punctuation at the end of the duplicate action error message to avoid run-on text. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. burr/core/graph.py:62
- Draft comment:
Ensure that the check for redundant default transitions using 'default' behaves as intended when multiple defaults exist. - 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.
3. burr/core/graph.py:342
- Draft comment:
In 'with_graph', consider checking for duplicate action names when extending actions and transitions to preempt later build errors. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. pyproject.toml:7
- Draft comment:
Version bump to 0.40.0rc0 appears appropriate; verify that all dependencies and optional groups are correctly specified. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to verify dependencies, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue.
5. tests/core/test_graph.py:89
- Draft comment:
Test for duplicated actions is solid, but consider adding tests for similar names with different casing to verify case-sensitivity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment suggests a reasonable edge case to test. However, without seeing the implementation of _validate_actions(), we don't know if case sensitivity is even a concern. The comment is speculative - it's asking "what if" without evidence that case sensitivity is an actual issue. We don't have enough context to know if this is a real concern.
I might be wrong about dismissing this - case sensitivity bugs can be serious issues in naming systems. The suggestion could prevent future bugs.
While case sensitivity is important, we should only keep comments that point to clear, evident issues. Without seeing the implementation, this is speculative.
Delete the comment as it's speculative and we don't have enough context to verify if case sensitivity testing is actually needed.
6. burr/core/graph.py:31
- Draft comment:
The error message for duplicate actions lacks proper spacing (e.g. between 'happen' and 'if'). Consider adding a space to improve readability. - 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%
While the comment is technically correct about the missing space, this is an extremely minor formatting issue in an error message that will only be seen when something goes wrong. The message is still perfectly understandable. The rules say not to make comments that are obvious or unimportant.
The missing space could make the error message harder to read when developers are trying to debug issues. Error messages should be as clear as possible.
While clear error messages are good, this is such a minor formatting issue that it doesn't meaningfully impact readability or understanding. The core message is still clear.
This comment should be removed as it addresses an extremely minor formatting issue that doesn't meaningfully impact code quality or functionality.
7. burr/core/graph.py:334
- Draft comment:
The with_graph() method correctly merges actions and transitions from an existing graph. It might be helpful to note in the docstring that duplicate action names will trigger an error during _validate_actions. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, as it is simply noting something about the code without suggesting any changes or asking for clarification. It doesn't align with the rules for useful comments.
8. tests/core/test_graph.py:90
- Draft comment:
The tests for graph builder functionality (including duplicate actions, transitions, and merging graphs) are comprehensive. Consider adding a comment to explain edge cases if needed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
9. burr/core/graph.py:76
- Draft comment:
The docstring in the _render_graphviz function reads: 'Use .pipe()` to not produce a not file; can't open the image'. This appears to be a typographical error. It likely meant to say something like 'to not produce a dot file' or similar. Please clarify and correct the wording. - 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.
10. burr/core/graph.py:265
- Draft comment:
The docstring for the GraphBuilder class refers to 'ApplicationBuidler', which appears to be a typographical error. It should likely be 'ApplicationBuilder'. - 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.
11. pyproject.toml:171
- Draft comment:
Typo: In the comment on line 171, consider changing "But its good for demos!" to "But it's good for demos!" to correctly reflect the contraction for "it is". - 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.
Workflow ID: wflow_HXpQV5TA2XHtDOIX
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.
burr/core/graph.py
Outdated
| if action.name in seen_action_names: | ||
| raise ValueError( | ||
| f"Action: {action.name} is duplicated in the actions list. " | ||
| "Please ensure all actions have unique names. This could happen" |
Contributor
There was a problem hiding this comment.
The error message for duplicate actions is built by concatenating multiple string literals. However, some segments miss a space. For example, the text following 'This could happen' is immediately joined with the next sentence (e.g., "This could happenif you add two actions..."). Please adjust the strings to include proper spacing.
Suggested change
| "Please ensure all actions have unique names. This could happen" | |
| "Please ensure all actions have unique names. This could happen " |
Contributor
There was a problem hiding this comment.
@elijahbenizzy it is right - also next lines too.
skrawcz
reviewed
Apr 7, 2025
pyproject.toml
Outdated
| [project] | ||
| name = "burr" | ||
| version = "0.39.1" | ||
| version = "0.40.0rc0" |
skrawcz
approved these changes
Apr 7, 2025
Contributor
skrawcz
left a comment
skrawcz
left a comment
There was a problem hiding this comment.
Just need to fix the nits. Also the commit / docs should be updated to mention this, and the caveat about edge ordering; you can't insert a higher priority edge condition in the merged graph.
elijahbenizzy
force-pushed
the
merge-graphs-application-builder
branch
from
11eb1f1 to
322cb2d
Compare
application builder Implementation of #526. Changes: 1. Adds with_graph(graph: Graph) to GraphBuilder -- this enables you to absorb another graph 2. Loosens up the mutually exclusive paths of with_graph and with_actions/transitions -- simplify to have one internal variable, allowing you to mix and match 3. Add tests for the API 4. Adds validation to ensure you don't have duplicated action names Note that this introduces the caveat of multiple duplicated action names, although the validation + fact that this is closer to a power-user feature makes it less problematic.
elijahbenizzy
force-pushed
the
merge-graphs-application-builder
branch
from
322cb2d to
ce3d801
Compare
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed ce3d801 in 1 minute and 34 seconds. Click for details.
- Reviewed
201lines of code in3files - Skipped
0files when reviewing. - Skipped posting
11draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. burr/core/graph.py:334
- Draft comment:
In with_graph, merging graphs by extending the actions and transitions lists is straightforward, but be aware that duplicate action names will trigger an error in _validate_actions. Consider documenting this behavior explicitly. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. tests/core/test_graph.py:118
- Draft comment:
Test for merging graphs (test_graph_builder_with_graph) is well-structured. Verify that merging two graphs with overlapping names raises a duplicate error as expected. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. burr/core/graph.py:31
- Draft comment:
Consider revising the error message string in _validate_actions to add explicit spaces between concatenated literals, ensuring the message is formatted as expected. - Reason this comment was not posted:
Marked as duplicate.
4. burr/core/graph.py:144
- Draft comment:
Ensure that condition.run(state) always returns a dictionary containing the key defined in Condition.KEY. Adding validation here could avoid potential KeyErrors. - 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.
5. burr/core/graph.py:268
- Draft comment:
Consider initializing 'actions' and 'transitions' as empty lists (non-Optional) in GraphBuilder.init to simplify later None-checks in methods like with_actions and with_graph. - 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.
6. tests/core/test_graph.py:132
- Draft comment:
Consider calling .build() on the GraphBuilder in test_graph_builder_with_graph to obtain and test the final Graph object directly, ensuring consistency between the builder's internal state and the built graph. - Reason this comment was not posted:
Comment looked like it was already resolved.
7. burr/core/application.py:1780
- Draft comment:
The error message in the block for single-step streaming actions is concatenated from two string literals without a space. For example,f"Action: {next_action.name} is not" + "an async action..."produces 'is notan async action'. Please add a space at the end of the first string or at the beginning of the second string. - 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.
8. burr/core/application.py:1240
- Draft comment:
The docstring for the asynchronous generator methodaiterateappears to be truncated (it ends with "...has no capability of t"). Please complete this sentence to clearly explain the method's 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. burr/core/graph.py:31
- Draft comment:
Typo: In the error message built in _validate_actions, the concatenation of string literals is missing necessary spaces. For example, between 'This could happen' and 'if you add two actions' there should be a space. Please revise the string literals to include proper spacing for readability. - Reason this comment was not posted:
Marked as duplicate.
10. burr/core/graph.py:76
- Draft comment:
Typo in _render_graphviz docstring: The phrase 'to not produce a not file' appears to be a mistake. It likely should read 'to not produce a dot file'. Please correct this 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.
11. burr/core/graph.py:265
- Draft comment:
Typo in GraphBuilder docstring: 'ApplicationBuidler' is misspelled. It should be 'ApplicationBuilder'. - 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.
Workflow ID: wflow_K8AeHIJTC8KRsH4A
You can customize 
| if action.name in seen_action_names: | ||
| raise ValueError( | ||
| f"Action: {action.name} is duplicated in the actions list. " | ||
| "actions have unique names. This could happen" |
Contributor
There was a problem hiding this comment.
Consider improving the formatting of the error message here. The concatenated strings lack proper spacing between sentences, e.g. add a space or punctuation after happen to separate it from the subsequent clause.
Suggested change
| "actions have unique names. This could happen" | |
| "actions have unique names. This could happen. " |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enables #526 -- this allows graph merging, and simplifies internal components of the codebase, by not storing a single graph in the graph builder. Note there are caveats here, but they are present in error messages.
Later on we may want to consider additional "namespacing" features -- E.G. rename the graphs + transitions, but for now this enables more modular graph construction.
Changes
How I tested this
Notes
Checklist
Important
Enhance
ApplicationBuilderto support merging multiple graphs and add validation for duplicate actions and redundant transitions.ApplicationBuildernow supports merging multiple graphs usingwith_graphs()andwith_graph()methods.prebuilt_graphattribute and related checks inapplication.py._validate_actions()ingraph.py.with_graph()method toGraphBuilderto merge existing graphs._validate_actions()ingraph.py._validate_transitions()ingraph.py.test_graph_builder_with_graph()intest_graph.pyto test graph merging.test__validate_actions_duplicated()intest_graph.pyto test duplicate action validation.This description was created by
for ce3d801. You can customize this summary. It will automatically update as commits are pushed.