Conversation
eakmanrq
force-pushed
the
eakmanrq/handle_indirect_cycles
branch
from
a74d964 to
7600b10
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR expands on a previous fix to handle indirect circular dependencies in DBT models by detecting cycles that are not created by direct dependencies. The fix improves error messages by providing exact cycle paths instead of candidate nodes.
- Added a new DFS-based function to find exact cycle paths in graphs
- Enhanced the DAG class to use precise cycle detection for better error messages
- Extended DBT model handling to detect and resolve indirect circular references through test dependencies
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| sqlmesh/utils/dag.py | Added find_path_with_dfs function for cycle detection and updated DAG error messages to show exact cycles |
| sqlmesh/dbt/basemodel.py | Enhanced circular reference detection to handle indirect cycles and improved test movement logic |
| tests/utils/test_dag.py | Updated test expectations to match new precise cycle error messages |
| tests/dbt/test_model.py | Added comprehensive tests for indirect circular reference detection and resolution |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
eakmanrq
force-pushed
the
eakmanrq/handle_indirect_cycles
branch
from
c5b124f to
2d6bf20
Compare
eakmanrq
changed the title
eakmanrq
force-pushed
the
eakmanrq/handle_indirect_cycles
branch
from
2d6bf20 to
93ce1e2
Compare
| cycle_msg = f"\nCycle: {' -> '.join(str(node) for node in cycle_path)} -> {cycle_path[0]}" | ||
| else: | ||
| last_processed_msg = "" | ||
| # Fallback message in case a cycle can't be found |
There was a problem hiding this comment.
Are there scenarios where a cycle won't be found? I'm wondering if we can remove the else.
There was a problem hiding this comment.
No known scenarios at this time but it seems like a safe fallback to have in place for now.
There was a problem hiding this comment.
Would it be better to not have a fallback to get user feedback on what scenario hits it more quickly?
Improve the cycle detection error message to output the exact cycle detected instead of potential candidates.