Internal refactoring for interface classes #636
Conversation
for more information, see https://pre-commit.ci
…utorbaseinterface
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExecutorInterface
participant ExecutorBase
User->>ExecutorInterface: initialize(executor)
ExecutorInterface->>ExecutorBase: delegate methods/properties
User->>ExecutorInterface: submit(fn, ...)
ExecutorInterface->>ExecutorBase: submit(fn, ...)
User->>ExecutorInterface: shutdown()
ExecutorInterface->>ExecutorBase: shutdown()
Poem
✨ Finishing Touches
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. 🪧 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #636 +/- ##
==========================================
+ Coverage 96.30% 96.65% +0.35%
==========================================
Files 28 28
Lines 1271 1287 +16
==========================================
+ Hits 1224 1244 +20
+ Misses 47 43 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (11)
tests/test_singlenodeexecutor_plot_dependency.py (6)
123-129: Same concern as above – the test relies on private attributes.
140-146: Same concern as above – the test relies on private attributes.
168-174: Same concern as above – the test relies on private attributes.
213-219: Same concern as above – the test relies on private attributes.
239-245: Same concern as above – the test relies on private attributes.
282-288: Same concern as above – the test relies on private attributes.executorlib/interfaces/flux.py (1)
365-385: Copy-before-mutate forresource_dictapplies here as well.executorlib/interfaces/slurm.py (4)
152-169: Copy-before-mutate forresource_dictapplies here as well.
171-186: Samedisable_dependencies/ validation asymmetry noted forFluxClusterExecutor.
329-345: Copy-before-mutate forresource_dictapplies here as well.
347-359: Validation asymmetry as inSingleNodeExecutor.
🧹 Nitpick comments (2)
executorlib/interfaces/single.py (1)
167-179: Validation asymmetry – plot/refresh checks bypassedWhen
disable_dependencies=Falsethe code path does not evaluatecheck_plot_dependency_graphnorcheck_refresh_rate.
AlthoughDependencyExecutormay validate internally, callers expect identical validation behaviour regardless of the flag.
Consider moving the two checks above theif not disable_dependencies:guard (or duplicating them in both branches) to keep behaviour consistent.executorlib/interfaces/flux.py (1)
191-197: Redundant per-class validation logic
check_plot_dependency_graph/check_refresh_rateare repeated across interface classes.
Extracting this into a tiny helper (e.g._validate_plot_config) would reduce duplication and the risk of inconsistent behaviour.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
executorlib/base/executor.py(2 hunks)executorlib/interfaces/flux.py(5 hunks)executorlib/interfaces/single.py(4 hunks)executorlib/interfaces/slurm.py(5 hunks)notebooks/4-developer.ipynb(1 hunks)tests/test_fluxjobexecutor_plot.py(4 hunks)tests/test_singlenodeexecutor_plot_dependency.py(7 hunks)tests/test_singlenodeexecutor_resize.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/test_fluxjobexecutor_plot.py (1)
executorlib/standalone/plot.py (1)
generate_nodes_and_edges(8-81)
tests/test_singlenodeexecutor_plot_dependency.py (1)
executorlib/standalone/plot.py (1)
generate_nodes_and_edges(8-81)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: unittest_mpich (macos-latest, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.12)
- GitHub Check: unittest_old
- GitHub Check: unittest_mpich (ubuntu-latest, 3.13)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.11)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: unittest_openmpi (macos-latest, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.12)
- GitHub Check: unittest_flux_openmpi
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: notebooks_integration
- GitHub Check: unittest_win
- GitHub Check: notebooks
- GitHub Check: unittest_flux_mpich
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
🔇 Additional comments (10)
executorlib/base/executor.py (2)
75-75: Good improvement to the API design.Making the
fnparameter positional-only with the/marker enforces that users must pass the function as a positional argument, preventing potential confusion in argument passing.
184-289: Well-designed interface abstraction.The
ExecutorInterfaceclass provides a clean implementation of the adapter pattern, forwarding methods and properties to the internal_task_schedulerinstance. This refactoring improves separation of concerns and maintainability.A few observations:
- The implementation correctly forwards all essential methods and properties
- The
submit()method properly handles the positional-only nature of thefnparameter- The class includes proper context manager support
This is a good architectural improvement that standardizes how executor interfaces interact with their implementation details.
tests/test_singlenodeexecutor_resize.py (2)
22-22: Test updated to match new architecture.The test has been correctly updated to access the
_processattribute through the new_task_schedulerindirection layer, consistent with the interface refactoring.
38-38: Test updated to match new architecture.The test has been correctly updated to access the
_processattribute through the new_task_schedulerindirection layer, consistent with the interface refactoring.notebooks/4-developer.ipynb (1)
83-83: Clear documentation update reflecting the new architecture.The documentation now correctly explains that the five executor interfaces internally map to task schedulers accessed via the
_task_schedulerattribute. This clarification aligns with the architectural changes and helps developers understand the internal structure.tests/test_fluxjobexecutor_plot.py (4)
56-57: Tests updated consistently with the new architecture.The tests have been correctly updated to access the internal dictionaries (
_future_hash_dictand_task_hash_dict) through the_task_schedulerattribute, maintaining consistency with the refactored interface design.Also applies to: 59-62
101-102: Tests updated consistently with the new architecture.The tests have been correctly updated to access the internal dictionaries (
_future_hash_dictand_task_hash_dict) through the_task_schedulerattribute, maintaining consistency with the refactored interface design.Also applies to: 104-107
127-128: Tests updated consistently with the new architecture.The tests have been correctly updated to access the internal dictionaries (
_future_hash_dictand_task_hash_dict) through the_task_schedulerattribute, maintaining consistency with the refactored interface design.Also applies to: 130-133
170-171: Tests updated consistently with the new architecture.The tests have been correctly updated to access the internal dictionaries (
_future_hash_dictand_task_hash_dict) through the_task_schedulerattribute, maintaining consistency with the refactored interface design.Also applies to: 173-176
executorlib/interfaces/flux.py (1)
346-364:disable_dependenciesparameter is forwarded but not validatedWhen
plot_dependency_graph=Falsethe code builds afile_executorand passesdisable_dependenciesthrough, but no local validation (check_plot_dependency_graph,check_refresh_rate) occurs. Ifdisable_dependenciesisTrue, these validations are skipped entirely. Align behaviour withFluxJobExecutor/SlurmJobExecutor.
| self.assertEqual(len(exe._task_scheduler._future_hash_dict), 2) | ||
| self.assertEqual(len(exe._task_scheduler._task_hash_dict), 2) | ||
| nodes, edges = generate_nodes_and_edges( | ||
| task_hash_dict=exe._task_hash_dict, | ||
| task_hash_dict=exe._task_scheduler._task_hash_dict, | ||
| future_hash_inverse_dict={ | ||
| v: k for k, v in exe._future_hash_dict.items() | ||
| v: k for k, v in exe._task_scheduler._future_hash_dict.items() | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid coupling tests to private executor internals
The tests now access exe._task_scheduler._future_hash_dict & exe._task_scheduler._task_hash_dict.
Because everything prefixed with “_” is considered private, any internal refactor (like the one in this PR) will break the tests again.
Consider exposing a thin, read-only property on ExecutorInterface (e.g. future_hash_dict / task_hash_dict) and let the tests consume that public API instead.
- self.assertEqual(len(exe._task_scheduler._future_hash_dict), 2)
- self.assertEqual(len(exe._task_scheduler._task_hash_dict), 2)
+ self.assertEqual(len(exe.future_hash_dict), 2)
+ self.assertEqual(len(exe.task_hash_dict), 2)If creating the properties is feasible, it will de-fragilize the tests without changing their intent.
🏁 Script executed:
#!/bin/bash
# Locate the ExecutorInterface definition
rg -n "class ExecutorInterface" -A5 .
# Locate the SingleNodeExecutor definition to see how it exposes scheduler internals
rg -n "class SingleNodeExecutor" -A5 .
# Check for any existing public accessors of future_hash_dict or task_hash_dict
rg -n "future_hash_dict" -n .
rg -n "task_hash_dict" -n .Length of output: 8077
🏁 Script executed:
#!/bin/bash
# Display ExecutorInterface class code
sed -n '160,260p' executorlib/base/executor.pyLength of output: 3112
🏁 Script executed:
#!/bin/bash
# Display the beginning of executorlib/interactive/dependency.py to inspect scheduler class and attributes
sed -n '1,200p' executorlib/interactive/dependency.pyLength of output: 6749
)
🏁 Script executed:
#!/bin/bash
# Display the beginning of SingleNodeExecutor implementation
sed -n '1,200p' executorlib/interfaces/single.pyLength of output: 12582
Add public accessors for task and future hash dictionaries
The tests in tests/test_singlenodeexecutor_plot_dependency.py (and similarly in test_fluxjobexecutor_plot) currently reach into the private scheduler internals:
- self.assertEqual(len(exe._task_scheduler._future_hash_dict), 2)
- self.assertEqual(len(exe._task_scheduler._task_hash_dict), 2)
+ self.assertEqual(len(exe.future_hash_dict), 2)
+ self.assertEqual(len(exe.task_hash_dict), 2)Because any refactor of _task_scheduler will break these assertions, please expose read-only properties on ExecutorInterface. For example, in executorlib/base/executor.py within the ExecutorInterface class, add:
@property
def future_hash_dict(self) -> dict:
"""Read-only access to the scheduler’s future hash mapping."""
return self._task_scheduler._future_hash_dict
@property
def task_hash_dict(self) -> dict:
"""Read-only access to the scheduler’s task hash mapping."""
return self._task_scheduler._task_hash_dictThis preserves test intent without coupling to private attributes.
| super().__init__( | ||
| executor=DependencyExecutor( | ||
| executor=create_single_node_executor( | ||
| max_workers=max_workers, | ||
| cache_directory=cache_directory, | ||
| max_cores=max_cores, | ||
| resource_dict=resource_dict, | ||
| hostname_localhost=hostname_localhost, | ||
| block_allocation=block_allocation, | ||
| init_function=init_function, | ||
| ), | ||
| max_cores=max_cores, | ||
| refresh_rate=refresh_rate, | ||
| plot_dependency_graph=plot_dependency_graph, | ||
| plot_dependency_graph_filename=plot_dependency_graph_filename, | ||
| ) | ||
| ) | ||
| else: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mutating caller-provided resource_dict can cause surprising side-effects
resource_dict.update(...) happens before the call to DependencyExecutor, which means if the caller passes in a dict that is reused elsewhere its content is silently modified.
Safeguard by copying the dict first:
- if resource_dict is None:
- resource_dict = {}
- resource_dict.update(
- {k: v for k, v in default_resource_dict.items() if k not in resource_dict}
- )
+ if resource_dict is None:
+ resource_dict = {}
+ else:
+ # copy to avoid side-effects outside this scope
+ resource_dict = resource_dict.copy()
+ resource_dict.update(
+ {k: v for k, v in default_resource_dict.items() if k not in resource_dict}
+ )Apply the same pattern wherever resource_dict is consumed (it recurs in many interface classes).
Committable suggestion skipped: line range outside the PR's diff.
| super().__init__( | ||
| executor=DependencyExecutor( | ||
| executor=create_flux_executor( | ||
| max_workers=max_workers, | ||
| cache_directory=cache_directory, | ||
| max_cores=max_cores, | ||
| resource_dict=resource_dict, | ||
| flux_executor=flux_executor, | ||
| flux_executor_pmi_mode=flux_executor_pmi_mode, | ||
| flux_executor_nesting=flux_executor_nesting, | ||
| flux_log_files=flux_log_files, | ||
| hostname_localhost=hostname_localhost, | ||
| block_allocation=block_allocation, | ||
| init_function=init_function, | ||
| ), | ||
| max_cores=max_cores, | ||
| refresh_rate=refresh_rate, | ||
| plot_dependency_graph=plot_dependency_graph, | ||
| plot_dependency_graph_filename=plot_dependency_graph_filename, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Copy resource_dict to maintain functional purity
Same mutability concern as raised for SingleNodeExecutor. Copy the user-supplied mapping before modification to protect callers from hidden side-effects.
Summary by CodeRabbit
Refactor
Documentation
Tests