-
Notifications
You must be signed in to change notification settings - Fork 8.4k
Fix: runflow toolmode input progation #11348
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: main
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughThis pull request refactors internal method naming and input handling in the RunFlowBaseComponent. Methods extracting tweaks are renamed to handle "ioputs", graph vertices are unfrozen after loading, error handling is added to cached flow execution, and output formatting is adjusted to skip non-terminal vertices. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
fffa1fe to
902d937
Compare
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.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/backend/tests/unit/base/tools/test_run_flow.py`:
- Around line 667-687: Remove the trailing whitespace in the test where
component._attributes is defined and add an assertion that _attributes is not
mutated by _build_flow_tweak_data: capture original_attributes =
dict(component._attributes), call component._build_flow_tweak_data(), then
assert component._attributes == original_attributes; if the implementation of
RunFlowBaseComponent._build_flow_tweak_data mutates _attributes, modify that
method to return merged tweak data without altering the original _attributes.
In `@src/lfx/src/lfx/base/tools/run_flow.py`:
- Around line 689-701: The method _build_flow_tweak_data mutates
self._attributes by assigning combined_values = self._attributes and calling
combined_values.update(tool_tweaks); instead, make combined_values a shallow
copy (e.g., copy dict(self._attributes) or use self._attributes.copy()) before
merging tool_tweaks so the original self._attributes is not modified, then
continue to call _extract_ioputs_from_keyed_values(combined_values); ensure you
also handle tool_tweaks.model_dump() as before and only update the copied
combined_values when tool_tweaks is a dict.
🧹 Nitpick comments (1)
src/lfx/src/lfx/base/tools/run_flow.py (1)
495-523: Graph mutation affects cached version; catch overly broad exceptions.The code caches the graph in
get_graph()and later passes it toprocess_tweaks_on_graph(), which mutates the graph in-place (iterates vertices and callsapply_tweaks_on_vertex()without copying). This means the cached version is modified, affecting subsequent executions. Consider adding a deepcopy before mutations or after retrieving from cache.The try/except block catches all exceptions with
# noqa: BLE001, potentially masking programming errors.get_graph()andrun_flow()explicitly raiseValueError, but catching all exceptions and silently returningNonecan hide unexpected issues during debugging. Consider catching specific exception types.
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (41.60%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #11348 +/- ##
==========================================
+ Coverage 34.84% 34.86% +0.02%
==========================================
Files 1420 1420
Lines 68200 68162 -38
Branches 9981 9974 -7
==========================================
+ Hits 23762 23765 +3
+ Misses 43213 43173 -40
+ Partials 1225 1224 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
56c8d7e to
67d3d2b
Compare
9a1b29d to
817e1d7
Compare
24c6257 to
2259e59
Compare
| self._attributes.pop("flow_tweak_data", None) | ||
|
|
||
| def _process_tweaks_on_graph(self, graph: Graph, tweaks: dict[str, dict[str, Any]]): | ||
| # there is a bug with the lfx process_tweaks_on_graph function |
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.
just note that im making a PR that not only fixes the bug but adds several integration tests to verify that the graph runtime behavior correctly reflects prior processing, for all processing helpers
2ac0bde to
8a5ce27
Compare
refactor runflow remove print calls allow Data inputs and refactoring of runflow component with updated tests misc misc persist tweaks in multi-input scenario add regression tests chore: update component index ruff (tests) checkout component index from main chore: update component index
8a5ce27 to
c4afb91
Compare
flow_tweak_datafromself._attributes.Message/Datainputs.Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.