-
Notifications
You must be signed in to change notification settings - Fork 308
(wip) Significant cleanup of BAML Graphs #2410
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
base: canary
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
🔒 Entelligence AI Vulnerability Scanner ✅ No security vulnerabilities found! Your code passed our comprehensive security analysis. |
LGTM 👍 |
🌿 Preview your docs: https://boundary-preview-98691528-bc41-4c2a-8f18-a2c61cc470bf.docs.buildwithfern.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to 9ec5b21 in 2 minutes and 38 seconds. Click for details.
- Reviewed
1648
lines of code in8
files - Skipped
0
files when reviewing. - Skipped posting
8
draft 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. engine/baml-lib/ast/examples/generate_mermaid_headers.rs:3
- Draft comment:
Good update – using 'diagram_generator' instead of the deprecated builder. Ensure example reflects the new API. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%
<= threshold50%
The comment is informative and suggests ensuring that examples reflect the new API. However, it doesn't ask for a specific test or code change, nor does it provide a specific suggestion or question about the code. It seems to be more of a reminder than a specific actionable comment.
2. engine/baml-lib/ast/src/ast.rs:32
- Draft comment:
Re-exporting the new 'diagram_generator' module replaces the old BamlVisDiagramGenerator – looks correct. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative and does not provide any actionable feedback or suggestions. It simply states that the re-export looks correct, which is not useful for the PR author.
3. engine/baml-lib/ast/src/ast/baml_vis.rs:18
- Draft comment:
Deprecated code for BamlVisDiagramGenerator has been removed in favor of the new module. This cleanup is clear. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, stating that deprecated code has been removed. It doesn't provide any actionable feedback or suggestions for improvement.
4. engine/baml-lib/ast/src/ast/baml_vis/graph.rs:604
- Draft comment:
Unreachable code detected in 'merge_by_pos': the early 'return' prevents the subsequent iterator implementation from ever executing. Consider removing the dead code. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% In Rust, when you define a type and implement traits for it within a function body, those definitions are part of the function's scope and are used to define the return type. Thereturn
statement here returns a value of typeState
, which needs the following definitions to be valid. This is not dead code - it's required for the function to work correctly. Could this be a case where the linter is correctly identifying that code after a return statement is technically unreachable, even if the code is needed for type definitions? No, this is a misunderstanding of Rust's scoping rules. The struct and impl blocks are declarations that are part of the function's scope and are required for the return type to be valid. They are not executable code that would be unreachable. The comment is incorrect. The code after the return statement contains necessary type definitions that are part of the function's scope and are required for the returned iterator type to work.
5. engine/baml-lib/baml/tests/mermaid_graph_tests.rs:4
- Draft comment:
Test update: Using 'diagram_generator' module now; snapshot tests align with the new API. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, providing details about a test update and alignment with a new API. It doesn't suggest any changes or ask for confirmation on specific code behavior.
6. engine/baml-runtime/src/runtime/runtime_interface.rs:175
- Draft comment:
Function 'function_graph' now uses 'diagram_generator::generate_headers_flowchart' correctly; verify that this integration reflects intended graph-generation behavior. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify the integration, which violates the rule against asking for confirmation of intention or behavior. It does not provide a specific suggestion or point out a specific issue with the code.
7. engine/baml-lib/ast/src/ast/baml_vis/graph.rs:107
- Draft comment:
Typo in comment: "precomuted" should be "precomputed". - 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 rules say not to make purely informative comments and to only comment if there is clearly a code change required. While this is technically a code change, it's an extremely minor typo fix in a doc comment that doesn't affect functionality. The rules suggest focusing on more substantive issues. The typo could theoretically cause confusion for future developers reading the docs. Documentation quality is important. While docs are important, this typo is extremely minor and obvious - any reader would understand the intended meaning. The cost of the review comment outweighs the tiny benefit. This comment should be deleted as it violates the rule about not making purely informative comments and focusing only on substantive issues that require code changes.
8. engine/baml-lib/ast/src/ast/baml_vis/graph.rs:240
- Draft comment:
Typo in comment: "wont" should be "want". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The rules state not to make purely informative comments and to avoid obvious or unimportant changes. While this is technically about a change (new file), fixing a typo in a comment has no functional impact and is a very minor issue. Comments should focus on code quality issues that require action. The typo could be confusing to future readers since "wont" is a real word with a different meaning than "want". Documentation clarity has value. While clear documentation is good, this is an internal comment about implementation details. The typo does not significantly impact understanding since the meaning is clear from context. The comment should be deleted as it points out a trivial issue that does not meaningfully impact code quality or functionality.
Workflow ID: wflow_ToyGhJNLECHQjSnd
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
🌿 Preview your docs: https://boundary-preview-2f74f435-6ddf-4892-aef1-07eda235335e.docs.buildwithfern.com |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
5eb1b9d
to
ec234fe
Compare
🌿 Preview your docs: https://boundary-preview-bc0ce846-4ef2-4613-a7f1-b74d3c43aaa1.docs.buildwithfern.com |
ec234fe
to
a0c9dc6
Compare
🌿 Preview your docs: https://boundary-preview-132cf8c6-0c3c-4eff-976d-5b4dc1b46def.docs.buildwithfern.com |
There's no need to make it live as much. remove get_by_hid guards in GraphBuilder We build a manual map that contains all entries. Why not just use that? make scope_root map local to `build` move nested_targets to `build` make cluster & node ids copyable Should relief a ton of copies. borrow strings from index Makes no sense to keep cloning all strings if all come from header index. Specially for call_node_cache! make pos_tuple return Path ref Another clone down :] make BamlVisDiagramGenerator a module baml_vis: move graph stuff under a module move mermaid diagram generator to a different function clippy + fix example Separate outputs from precompute() as cached graph data We can now use a new lifetime to separate read & write locks inside the struct. Do not clone `nested_chlidren` on each `get` `.clone()` as an escape hatch is a big mistake. annotate & arrange processing order (dependency lists) Only done for if branches. Should be done for the rest, slowly. Also removed a couple of inefficiencies wrt collect. Make `build_header` return unit Apart from removing some clones, this is to separate the visit process from the data so it is easier to extract phases. Visit header nodes only once The cache check was hiding bugs! Visits are pre/post-order through a tree, so we can't possibly visit a node twice by following edges. remove visited_scopes Now that we make sure to visit nodes only once, it proves unnecessary. move render_header_calls out of recursive visit remove call_node_cache Now we can easily prove that each (hid, callee) pair is only visited once! move node & cluster id creation to graph. separate adding graph edges from recursive visit move baml_vis stuff to baml_vis; make scope traversal always source order simplify getting top-level scopes add header filter Now the recursion uses the result from the header filter.
0c773b6
to
4c72097
Compare
Taking a look at how the current implementation of BAML Graphs works, & working
through data separation to make DX & understanding the pipeline easier.
So far:
.clone()
where a&
is enough. Missing a couple of places due to required lifetime separations.HeaderIndex
usefulness is indeterminate. The tree structure it contains is not homogeneous enoughto make for direct traversal, & that shows up in
GraphBuilder
.Graph
is a useful but not-needed yet separation.Important
Refactor BAML Graphs by modularizing diagram generation, simplifying graph logic, and renaming for clarity.
BamlVisDiagramGenerator
logic todiagram_generator
module inbaml_vis
.graph
logic intograph.rs
for better separation of concerns.BamlVisDiagramGenerator
todiagram_generator
in imports and usage acrossgenerate_mermaid_headers.rs
,runtime_interface.rs
, andmermaid_graph_tests.rs
..clone()
calls where&
references suffice.SHOW_CALL_NODES
constant usage indiagram_generator.rs
andgraph.rs
.This description was created by
for 9ec5b21. You can customize this summary. It will automatically update as commits are pushed.