[feature] Add support for future objects in input dictionaries #568
Conversation
WalkthroughThe changes enhance the handling of dictionary inputs by adding recursive processing for future objects. In the shared library, functions such as Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant get_result
participant Iteration
participant FutureEvaluator
Caller->>+get_result: Call with input (possibly a dictionary)
alt Input is a dictionary
get_result->>+Iteration: Iterate over key/value pairs
Iteration-->>-get_result: Each value processed recursively
get_result->>+FutureEvaluator: Evaluate future object if found
FutureEvaluator-->>-get_result: Return computed value
else Input is not a dictionary
get_result-->>Caller: Return value as-is
end
get_result-->>Caller: Return fully evaluated result
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_dependencies_executor.py (1)
137-143: Enhance test coverage for nested dictionaries.While the test verifies basic dictionary handling with future objects, consider adding test cases for:
- Nested dictionaries with multiple levels of future objects
- Dictionaries with multiple future objects at different levels
- Empty dictionaries
- Dictionaries with mixed types (futures, lists, nested dicts)
Example test cases:
def test_future_input_dict_nested(self): with SingleNodeExecutor() as exe: fs = exe.submit( return_input_dict, input_dict={ "a": exe.submit(sum, [2, 2]), "b": {"c": exe.submit(sum, [3, 3])}, "d": [], "e": {} }, ) result = fs.result() self.assertEqual(result["a"], 4) self.assertEqual(result["b"]["c"], 6) self.assertEqual(result["d"], []) self.assertEqual(result["e"], {})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
executorlib/interactive/shared.py(2 hunks)tests/test_dependencies_executor.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.10)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.12)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.10)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.11)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: unittest_win
- GitHub Check: unittest_mpich (ubuntu-latest, 3.12)
- GitHub Check: unittest_openmpi (macos-latest, 3.13)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.13)
- GitHub Check: unittest_old
- GitHub Check: unittest_mpich (macos-latest, 3.13)
- GitHub Check: unittest_flux_openmpi
- GitHub Check: unittest_flux_mpich
- GitHub Check: notebooks
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
🔇 Additional comments (3)
tests/test_dependencies_executor.py (1)
41-42: LGTM!Simple helper function that returns its input dictionary unchanged, used for testing purposes.
executorlib/interactive/shared.py (2)
486-487: LGTM!The implementation correctly handles dictionaries by recursively processing their values, maintaining consistency with the existing list handling.
515-516: LGTM!The implementation correctly extends future object detection to dictionary values, maintaining consistency with the existing list handling.
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #568 +/- ##
==========================================
+ Coverage 95.73% 95.80% +0.06%
==========================================
Files 26 26
Lines 1149 1168 +19
==========================================
+ Hits 1100 1119 +19
Misses 49 49 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
executorlib/standalone/plot.py (1)
42-63: LGTM! Consider adding type hints for better code maintainability.The implementation for handling dictionaries containing Future objects is well-structured and follows the same pattern as list handling. The approach of creating placeholder dictionaries and linking Future values is clean and consistent.
Consider adding type hints to improve code maintainability:
-def add_element(arg, link_to, label=""): +def add_element(arg: Any, link_to: int, label: str = "") -> None:tests/test_plot_dependency.py (1)
42-43: LGTM! Consider adding a docstring for better documentation.The helper function is simple and serves its purpose well.
Add a docstring to improve documentation:
def return_input_dict(input_dict): + """ + Helper function that returns the input dictionary unchanged. + Used for testing dictionary handling with Future objects. + + Args: + input_dict (dict): The input dictionary to return + + Returns: + dict: The unchanged input dictionary + """ return input_dict
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
executorlib/standalone/plot.py(2 hunks)tests/test_plot_dependency.py(4 hunks)tests/test_plot_dependency_flux.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
🔇 Additional comments (4)
executorlib/standalone/plot.py (1)
114-118: LGTM! Clean implementation of dictionary support in hash generation.The recursive conversion of dictionary values is well-implemented and maintains consistency with the existing list handling pattern.
tests/test_plot_dependency_flux.py (1)
109-110: LGTM! Assertion updates reflect the new graph structure.The updated node and edge counts are consistent across test classes and accurately reflect the changes in graph structure due to dictionary support.
Also applies to: 178-179
tests/test_plot_dependency.py (2)
134-149: LGTM! Comprehensive test coverage for dictionary handling.The test case effectively validates the handling of Future objects within dictionaries, checking both the execution flow and the generated dependency graph structure.
131-132: LGTM! Consistent assertion updates across test classes.The updated node and edge counts are consistent across all test classes, maintaining test integrity for the new dictionary support feature.
Also applies to: 221-222, 290-291
Summary by CodeRabbit