Skip to content

Conversation

@panicall
Copy link

@panicall panicall commented Nov 23, 2025

BUG Description

When entering a new cycle, the whole predecessor_map values will be set to empty list.
function call: component.start() --> graph.mark_branch() --> update_run_state()
‘’‘python
new_predecessor_map = {
k: [dep for dep in v if dep in self.cycle_vertices and dep in self.run_manager.ran_at_least_once]
for k, v in new_predecessor_map.items()
}
‘’‘
At the point, self.run_manager.ran_at_least_once list is almost empty so that new_predecessor_map will also be values-empty.

This is a BIG bug, during my cycle, the execution order of each node is chaotic, and even some nodes are executed twice.

Fix

remove 'dep in self.run_manager.ran_at_least_once' since every node in the cycle should be rebuilt according to different condition.

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of circular dependencies by including all relevant predecessors in execution flow calculations, ensuring proper run state updates and execution sequencing.

✏️ Tip: You can customize this high-level summary in your review settings.

@github-actions github-actions bot added the community Pull Request from an external contributor label Nov 23, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 23, 2025

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

A filter condition in the Graph.mark_branch method is removed when handling predecessor dependencies in cycles. The change broadens which predecessors remain in the dependency map by no longer filtering for the run_at_least_once state, affecting execution order during cycle handling.

Changes

Cohort / File(s) Summary
Cycle predecessor filtering
src/lfx/src/lfx/graph/graph/base.py
Removed run_at_least_once filter when selecting predecessor dependencies in cycles, retaining all predecessors that are in the cycle instead of only those that have executed at least once

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Verify the intent: why is removing the run_at_least_once filter the correct behavior for cycle handling?
  • Check for any test coverage that validates this cycle predecessor behavior
  • Confirm that this change doesn't introduce unintended execution order side effects or deadlocks in cyclic dependencies

Pre-merge checks and finishing touches

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 warning, 3 inconclusive)
Check name Status Explanation Resolution
Test Coverage For New Implementations ❌ Error The PR modifies cycle handling logic in mark_branch method but contains no test files or test coverage additions in the repository. Add test files (test_*.py) that specifically test the mark_branch cycle handling logic and predecessor map reconstruction to provide regression coverage.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Test Quality And Coverage ❓ Inconclusive Repository source code cannot be accessed in sandbox environment due to clone operation failures, preventing evaluation of tests for mark_branch method changes. Ensure repository accessibility and retry evaluation to examine test directory structure and verify tests cover cycle detection logic and filter removal scenarios.
Test File Naming And Structure ❓ Inconclusive Assessment cannot be completed because the repository is not accessible in the current environment, preventing verification of test files, naming patterns, pytest structure, and edge case coverage. Ensure the repository is accessible and cloned successfully, then verify test naming conventions, pytest structure, comprehensive coverage, and edge case handling for cycle handling and predecessor mapping.
Excessive Mock Usage Warning ❓ Inconclusive Assessment of mock usage in tests cannot be completed due to inability to access repository codebase and test files. Restore repository access or provide test files directly (particularly those testing graph module's mark_branch and new_predecessor_map functionality) for mock usage pattern review.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: removing the run_at_least_once filter from new_predecessor_map to fix error handling in cycle detection.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added bug Something isn't working and removed bug Something isn't working labels Nov 23, 2025
@github-actions github-actions bot added bug Something isn't working and removed bug Something isn't working labels Nov 23, 2025
@github-actions github-actions bot added bug Something isn't working and removed bug Something isn't working labels Nov 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working community Pull Request from an external contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant