Implement resource_dict for file executor #456
Conversation
|
Warning Rate limit exceeded@jan-janssen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces significant changes to the resource management approach within the executor library. The Changes
Possibly related PRs
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: 6
🧹 Outside diff range and nitpick comments (6)
executorlib/cache/executor.py (1)
39-47: Consider adding type validation for resource_dict values.The resource dictionary initialization logic is well-implemented, preserving user-provided values while ensuring defaults. However, consider adding type validation for the dictionary values to catch configuration errors early.
Consider adding type validation:
if resource_dict is None: resource_dict = {} + # Validate resource types + if "cores" in resource_dict and not isinstance(resource_dict["cores"], int): + raise TypeError("cores must be an integer") + if "cwd" in resource_dict and not (isinstance(resource_dict["cwd"], str) or resource_dict["cwd"] is None): + raise TypeError("cwd must be a string or None") resource_dict.update( {k: v for k, v in default_resource_dict.items() if k not in resource_dict} )tests/test_cache_executor_serial.py (2)
75-75: Consider simplifying the resource specification.The test case doesn't appear to require specific resource constraints, yet it sets both
coresandcwd. Consider whether this level of specification is necessary for this test case.- "resource_dict": {"cores": 1, "cwd": None}, + "resource_dict": {},
Line range hint
33-33: Consider adding tests for resource_dict edge cases.The test suite would benefit from additional test cases that verify:
- Invalid resource_dict values
- Resource inheritance between dependent tasks
- Resource override scenarios
Would you like me to provide example test cases for these scenarios?
executorlib/interactive/executor.py (2)
178-180: Documentation needs clarification and consistency fixes.The resource_dict documentation has the following issues:
- The description for
gpus_per_corestill references "per worker" which contradicts the new parameter name- The rationale for changing from worker-based to core-based resource allocation isn't explained
Consider updating the documentation:
- - cores (int): number of MPI cores to be used for each function call - - gpus_per_core (int): number of GPUs per worker - defaults to 0 + - cores (int): number of MPI cores to be used for each function call + - gpus_per_core (int): number of GPUs allocated per core - defaults to 0
Line range hint
209-209: Add validation for required resource_dict keys.The direct access to
resource_dict["cores"]could raise a KeyError if the key is missing. Consider adding validation for required keys.+ required_keys = ["cores", "threads_per_core", "gpus_per_core", "cwd", "openmpi_oversubscribe", "slurm_cmd_args"] + missing_keys = [key for key in required_keys if key not in resource_dict] + if missing_keys: + raise ValueError(f"Missing required keys in resource_dict: {missing_keys}") cores_per_worker = resource_dict["cores"]executorlib/__init__.py (1)
Line range hint
176-183: LGTM with a minor suggestion for improvement.The implementation correctly handles the new resource_dict keys and maintains proper default values. Consider extracting the default_resource_dict to a module-level constant to make it reusable and easier to maintain.
+# At module level +DEFAULT_RESOURCE_DICT = { + "cores": 1, + "threads_per_core": 1, + "gpus_per_core": 0, + "cwd": None, + "openmpi_oversubscribe": False, + "slurm_cmd_args": [], +} def __new__( cls, ... ): - default_resource_dict = { - "cores": 1, - "threads_per_core": 1, - "gpus_per_core": 0, - "cwd": None, - "openmpi_oversubscribe": False, - "slurm_cmd_args": [], - } 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} + {k: v for k, v in DEFAULT_RESOURCE_DICT.items() if k not in resource_dict} )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- executorlib/init.py (1 hunks)
- executorlib/cache/executor.py (4 hunks)
- executorlib/cache/shared.py (4 hunks)
- executorlib/interactive/executor.py (1 hunks)
- tests/test_cache_executor_mpi.py (1 hunks)
- tests/test_cache_executor_pysqa_flux.py (1 hunks)
- tests/test_cache_executor_serial.py (4 hunks)
🔇 Additional comments (10)
tests/test_cache_executor_mpi.py (1)
32-32: LGTM! Correct implementation of resource_dict parameter.The change from
cores_per_worker=2toresource_dict={"cores": 2}aligns with the PR objective of unifying resource specification.tests/test_cache_executor_pysqa_flux.py (2)
34-38: LGTM! Test implementation is correct.The test correctly validates the parallel execution behavior with the new resource_dict parameter while maintaining the existing test logic for verifying MPI functionality.
35-35: Verify resource_dict parameter consistency across test files.The change from
cores_per_workertoresource_dict={"cores": 2}aligns with the PR objective. Let's verify this pattern is consistently applied across all test files.✅ Verification successful
Parameter change is consistently implemented across test files
The verification shows that:
- No remaining usage of
cores_per_workerparameter exists in test filesresource_dictwithcoresparameter is consistently used across all test files- The implementation in
test_cache_executor_pysqa_flux.pyaligns with the pattern used in other test files🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of resource_dict parameter across test files # Expected: All FileExecutor instantiations should use resource_dict instead of cores_per_worker # Search for any remaining usage of cores_per_worker in test files echo "Checking for any remaining cores_per_worker usage:" rg "cores_per_worker" "tests/" # Search for resource_dict usage pattern in test files echo -e "\nVerifying resource_dict usage pattern:" rg "resource_dict.*cores.*\d+" "tests/"Length of output: 1976
executorlib/cache/executor.py (2)
17-17: LGTM: Constructor signature change is well-implemented.The new
resource_dictparameter is properly typed asOptional[dict]and effectively consolidates resource-related parameters.
59-59: Verify execute_tasks_h5 compatibility with resource_dict.The resource_dict is correctly passed to execute_tasks_h5, but we should verify that the function has been updated to handle this parameter correctly.
Let's verify the execute_tasks_h5 implementation:
✅ Verification successful
execute_tasks_h5 is properly updated to handle resource_dict parameter
The verification shows that
execute_tasks_h5inexecutorlib/cache/shared.pyis correctly defined withresource_dictas a required parameter in its signature. The function's documentation also indicates it expects a dictionary of resources. The test files demonstrate proper usage of the parameter by passing resource dictionaries with expected keys.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if execute_tasks_h5 function is updated to handle resource_dict # Search for execute_tasks_h5 function definition and its usage of resource_dict ast-grep --pattern 'def execute_tasks_h5($$$) { $$$ resource_dict $$$ }'Length of output: 74
Script:
#!/bin/bash # Let's try a different approach to find the execute_tasks_h5 function and its implementation # First, let's find the file containing execute_tasks_h5 rg "execute_tasks_h5" -l # Then get the full context of the function rg "execute_tasks_h5" -B 5 -A 15Length of output: 9445
tests/test_cache_executor_serial.py (1)
51-51: LGTM! Verify resource_dict format consistency.The change to use resource_dict for working directory specification aligns with the PR's objective of unified resource management.
Let's verify the resource_dict format consistency across the codebase:
✅ Verification successful
Resource dictionary usage is consistent across the codebase
The verification shows that:
resource_dictis an optional parameter in theFileExecutorclass- All instances of
FileExecutorin the codebase either:
- Use no resources (default constructor)
- Properly use the
resource_dictparameter with specific resources ("cwd", "cores")- The format is consistent across different test files (test_cache_executor_serial.py, test_cache_executor_pysqa_flux.py, test_cache_executor_mpi.py)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all FileExecutor instantiations to ensure consistent resource_dict usage ast-grep --pattern 'FileExecutor($$$resource_dict=$$$)'Length of output: 57
Script:
#!/bin/bash # Let's try a broader search for FileExecutor instantiations rg "FileExecutor\(" -A 2 # Also search for the class definition to understand the parameters rg "class FileExecutor"Length of output: 1532
Script:
#!/bin/bash # Let's check the FileExecutor class implementation to understand the resource_dict parameter rg -A 10 "class FileExecutor" executorlib/cache/executor.pyLength of output: 434
executorlib/interactive/executor.py (1)
Line range hint
209-267: Standardize resource handling across backends.The resource dictionary handling differs across backends:
- Local backend removes threads_per_core and gpus_per_core
- Flux backend removes openmpi_oversubscribe and slurm_cmd_args
- Slurm backend keeps all keys
This inconsistency could lead to confusion and maintenance issues.
Consider:
- Standardizing which keys are required vs optional for each backend
- Documenting backend-specific requirements in the docstring
- Implementing a validation function for each backend's resource requirements
Let's check if this inconsistency exists in other executor implementations:
executorlib/__init__.py (1)
141-143: Verify documentation for breaking changes in resource_dict keys.The renaming of resource_dict keys from 'cores_per_worker'/'gpus_per_worker' to 'cores'/'gpus_per_core' represents a breaking change that could affect existing code. While the new names are more precise, we should ensure these changes are properly documented.
executorlib/cache/shared.py (2)
122-122: Verify the usage ofresource_dictwhen callingexecute_functionAt line 122,
resource_dict=task_resource_dictis passed toexecute_function. Ensure thatexecute_functioncan handle cases wheretask_resource_dictmight be empty or missing expected keys.Confirm that
execute_functiongracefully handles incompleteresource_dictinputs.
Line range hint
53-57: Ensure all calls toexecute_tasks_h5are updated with the newresource_dictparameterThe function
execute_tasks_h5now requires aresource_dictparameter instead ofcores_per_workerandcwd. Please verify that all calls to this function throughout the codebase have been updated to reflect this change.Run the following script to search for any outdated function calls:
✅ Verification successful
All calls to
execute_tasks_h5are correctly using the newresource_dictparameterThe verification shows that all calls to
execute_tasks_h5have been properly updated:
- In
tests/test_cache_executor_serial.py, all calls useresource_dictwith{"cores": 1, "cwd": None}- In
executorlib/cache/executor.py, the call correctly passes theresource_dictparameterNo outdated calls using
cores_per_workerorcwdparameters were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of execute_tasks_h5 with old parameters # Expectation: No matches should be found rg 'execute_tasks_h5\([^\)]*(cores_per_worker|cwd)\s*='Length of output: 57
Script:
#!/bin/bash # Let's first find all files that contain 'execute_tasks_h5' to analyze its usage rg -l "execute_tasks_h5" # Then let's see the actual calls with context rg "execute_tasks_h5" -B 2 -A 2Length of output: 2475
Script:
#!/bin/bash # Let's examine the actual calls in test_cache_executor_serial.py to see the parameters being passed rg -A 10 "target=execute_tasks_h5" tests/test_cache_executor_serial.py # Also check the call in executor.py rg -A 10 "target=execute_tasks_h5" executorlib/cache/executor.pyLength of output: 1839
tests/test_cache_executor_mpi.py
Outdated
| with FileExecutor(resource_dict={"cores": 2}) as exe: | ||
| fs1 = exe.submit(mpi_funct, 1) | ||
| self.assertFalse(fs1.done()) | ||
| self.assertEqual(fs1.result(), [(1, 2, 0), (1, 2, 1)]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding test cases for resource_dict validation.
While the basic functionality is tested, consider adding test cases that verify:
- Invalid resource dictionary formats
- Missing required resources
- Additional resource parameters that FileExecutor supports
Example test case structure:
def test_executor_resource_validation(self):
# Test invalid cores value
with self.assertRaises(ValueError):
with FileExecutor(resource_dict={"cores": -1}):
pass
# Test missing required resources
with self.assertRaises(KeyError):
with FileExecutor(resource_dict={}):
pass| resource_dict (dict): A dictionary of resources required by the task. With the following keys: | ||
| - cores (int): number of MPI cores to be used for each function call | ||
| - cwd (str/None): current working directory where the parallel python task is executed |
There was a problem hiding this comment.
Remove documentation for deprecated parameters.
The docstring still contains entries for the removed parameters cores_per_worker and cwd. These should be removed since they've been replaced by resource_dict.
Apply this diff to fix the docstring:
resource_dict (dict): A dictionary of resources required by the task. With the following keys:
- cores (int): number of MPI cores to be used for each function call
- cwd (str/None): current working directory where the parallel python task is executed
execute_function (callable, optional): The function to execute tasks. Defaults to execute_in_subprocess.
- cores_per_worker (int, optional): The number of CPU cores per worker. Defaults to 1.
terminate_function (callable, optional): The function to terminate the tasks.
- cwd (str, optional): current working directory where the parallel python task is executedAlso applies to: 33-34
| @@ -113,7 +113,7 @@ def test_executor_function_dependence_kwargs(self): | |||
| "future_queue": q, | |||
| "cache_directory": cache_dir, | |||
| "execute_function": execute_in_subprocess, | |||
| "cores_per_worker": 1, | |||
| "resource_dict": {"cores": 1, "cwd": None}, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring duplicate resource configurations.
The same resource configuration {"cores": 1, "cwd": None} is repeated across multiple test methods. Consider extracting this to a class-level constant or setUp method to improve maintainability.
class TestCacheExecutorSerial(unittest.TestCase):
+ @classmethod
+ def setUpClass(cls):
+ super().setUpClass()
+ cls.default_resource_dict = {"cores": 1, "cwd": None}
def test_executor_function(self):
# ... existing code ...
kwargs={
"future_queue": q,
"cache_directory": cache_dir,
"execute_function": execute_in_subprocess,
- "resource_dict": {"cores": 1, "cwd": None},
+ "resource_dict": self.default_resource_dict,
"terminate_function": terminate_subprocess,
}Also applies to: 157-157
| @@ -115,12 +114,12 @@ def execute_tasks_h5( | |||
| process_dict[task_key] = execute_function( | |||
| command=_get_execute_command( | |||
| file_name=file_name, | |||
| cores=cores_per_worker, | |||
| cores=task_resource_dict["cores"], | |||
There was a problem hiding this comment.
Ensure 'cores' key exists in task_resource_dict before access
At line 117, accessing task_resource_dict["cores"] without checking if the 'cores' key exists may raise a KeyError if it is missing.
Consider providing a default value or adding a check:
Option 1: Provide a default value for 'cores':
- cores=task_resource_dict["cores"],
+ cores=task_resource_dict.get("cores", 1),Option 2: Add a check and raise an explicit error if 'cores' is missing:
if "cores" not in task_resource_dict:
raise KeyError("'cores' key is missing in task_resource_dict")
cores = task_resource_dict["cores"]| resource_dict (dict): A dictionary of resources required by the task. With the following keys: | ||
| - cores (int): number of MPI cores to be used for each function call | ||
| - cwd (str/None): current working directory where the parallel python task is executed |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve docstring formatting for resource_dict parameter
The docstring for resource_dict can be reformatted for clarity and to adhere to standard documentation practices.
Consider applying this change:
- resource_dict (dict): A dictionary of resources required by the task. With the following keys:
- - cores (int): number of MPI cores to be used for each function call
- - cwd (str/None): current working directory where the parallel python task is executed
+ resource_dict (dict): A dictionary of resources required by the task with the following keys:
+ - **cores** (int): Number of MPI cores to be used for each function call.
+ - **cwd** (Optional[str]): Current working directory where the parallel Python task is executed.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resource_dict (dict): A dictionary of resources required by the task. With the following keys: | |
| - cores (int): number of MPI cores to be used for each function call | |
| - cwd (str/None): current working directory where the parallel python task is executed | |
| resource_dict (dict): A dictionary of resources required by the task with the following keys: | |
| - **cores** (int): Number of MPI cores to be used for each function call. | |
| - **cwd** (Optional[str]): Current working directory where the parallel Python task is executed. |
| task_resource_dict = task_dict["resource_dict"].copy() | ||
| task_resource_dict.update( | ||
| {k: v for k, v in resource_dict.items() if k not in task_resource_dict} | ||
| ) |
There was a problem hiding this comment.
Handle missing 'resource_dict' in task_dict to prevent KeyError
At line 100, the code assumes that task_dict always contains a 'resource_dict' key. If this key is missing, it will raise a KeyError.
Apply this diff to safely handle cases where 'resource_dict' might not be present:
-task_resource_dict = task_dict["resource_dict"].copy()
+task_resource_dict = task_dict.get("resource_dict", {}).copy()Additionally, ensure that the subsequent code accounts for the possibility of an empty task_resource_dict.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| task_resource_dict = task_dict["resource_dict"].copy() | |
| task_resource_dict.update( | |
| {k: v for k, v in resource_dict.items() if k not in task_resource_dict} | |
| ) | |
| task_resource_dict = task_dict.get("resource_dict", {}).copy() | |
| task_resource_dict.update( | |
| {k: v for k, v in resource_dict.items() if k not in task_resource_dict} | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests/test_cache_hdf.py (1)
19-19: Minor: Consider rephrasing the skip message.While the message is accurate, it could be more concise.
Consider this alternative:
- skip_h5py_test, "h5py is not installed, so the h5io tests are skipped." + skip_h5py_test, "Skipping HDF tests: h5py package not installed"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- executorlib/cache/executor.py (3 hunks)
- tests/test_cache_executor_mpi.py (2 hunks)
- tests/test_cache_executor_pysqa_flux.py (1 hunks)
- tests/test_cache_executor_serial.py (6 hunks)
- tests/test_cache_hdf.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_cache_executor_mpi.py
- tests/test_cache_executor_pysqa_flux.py
🔇 Additional comments (11)
tests/test_cache_hdf.py (2)
9-9: LGTM! Variable rename improves clarity.The rename from
skip_h5io_testtoskip_h5py_testbetter reflects that we're checking for the availability of the h5py package.Also applies to: 11-11
Line range hint
22-78: Test coverage looks comprehensive.The test suite thoroughly covers different argument patterns (mixed, args-only, kwargs-only) for HDF operations. The unchanged test cases continue to provide good coverage of the core functionality.
executorlib/cache/executor.py (3)
23-24: LGTM! Constructor signature changes are well-structured.The updated signature with
resource_dictand new default executor aligns well with the architectural changes.
12-16: Verify pysqa availability in deployment environments.The change of default executor to
execute_with_pysqawith fallback toexecute_in_subprocessmight affect existing deployments.✅ Verification successful
pysqais an optional dependency, fallback mechanism is appropriateThe verification reveals that
pysqais properly configured as an optional dependency in thequeueextras group withinpyproject.toml. The fallback toexecute_in_subprocessis a well-designed pattern that ensures the library remains functional even withoutpysqainstalled. Users who need queue system integration can explicitly install it usingpip install executorlib[queue].Key findings:
pysqais specified as version0.2.0in the optionalqueuedependencies- The
execute_with_pysqaimplementation provides queue system integration when available- The fallback mechanism maintains backward compatibility for basic usage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if pysqa is listed as a dependency # Test: Look for pysqa in requirements files rg -l 'pysqa' --glob '{requirements,setup.py,pyproject.toml}'Length of output: 178
Script:
#!/bin/bash # Check for pysqa related imports and usage across the codebase rg -l "pysqa" # Check setup.py specifically for optional dependencies fd "setup.py" --exec cat {} # Look for any documentation about optional dependencies fd "README" --extension md --exec cat {}Length of output: 8472
Script:
#!/bin/bash # Check pyproject.toml for pysqa dependency configuration cat pyproject.toml # Check the implementation of execute_with_pysqa to understand its role cat executorlib/standalone/cache/queue.pyLength of output: 3161
63-63: Verify execute_tasks_h5 compatibility with resource_dict.Ensure that the
execute_tasks_h5function has been updated to handle the newresource_dictparameter.✅ Verification successful
The execute_tasks_h5 function is properly updated to handle resource_dict parameter
Based on the search results, the
execute_tasks_h5function inexecutorlib/cache/shared.pyis already properly updated to handle theresource_dictparameter:
- The function signature includes
resource_dictas a required parameter:def execute_tasks_h5( future_queue: queue.Queue, cache_directory: str, execute_function: callable, resource_dict: dict, ... )
- The function's docstring properly documents the parameter:
resource_dict (dict): A dictionary of resources required by the task. With the following keys: - cores (int): number of MPI cores to be used for each function call - cwd (str/None): current working directory where the parallel python task is executed
- The function correctly uses the parameter in its implementation:
task_resource_dict = task_dict["resource_dict"].copy() task_resource_dict.update( {k: v for k, v in resource_dict.items() if k not in task_resource_dict} )🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if execute_tasks_h5 is updated to handle resource_dict ast-grep --pattern 'def execute_tasks_h5($params) { $$$ resource_dict $$$ }'Length of output: 178
Script:
#!/bin/bash # Search for execute_tasks_h5 function definition and its usage of resource_dict rg "def execute_tasks_h5" -A 20Length of output: 1648
Script:
#!/bin/bash # Search for resource_dict usage in the codebase rg "resource_dict" -A 3Length of output: 37378
tests/test_cache_executor_serial.py (6)
7-10: LGTM! Import organization improvement.The reorganization of imports improves code clarity by grouping related imports together.
17-17: LGTM! More accurate test skip condition naming.The rename from
skip_h5io_testtoskip_h5py_testbetter reflects the actual dependency being checked.Also applies to: 19-19, 31-31
35-35: LGTM! Explicit executor function specification.The addition of
execute_function=execute_in_subprocessmakes the test behavior more explicit and aligns with the executor library changes.Also applies to: 42-42
51-53: LGTM! Proper resource_dict implementation.The change from direct
cwdparameter toresource_dictformat aligns with the new resource management approach while maintaining the same test coverage.
77-77: Duplicate resource configuration issue.A previous review comment already addresses the duplication of resource configuration across test methods.
Also applies to: 118-118, 159-159
Line range hint
1-173: Verify complete migration to resource_dict.Let's ensure all resource-related parameters have been migrated to the new resource_dict format.
✅ Verification successful
Let's examine the specific usage of these parameters in the relevant files to determine if there are any unconverted instances.
Migration to resource_dict is properly implemented
The code review shows that the migration to resource_dict is complete and consistent:
- Core files show proper resource_dict parameter handling
- Direct core/cwd parameters are only used internally after being extracted from resource_dict
- All test files and implementation files follow the new resource_dict pattern
- The remaining direct parameter references are either:
- Inside the resource_dict itself (e.g.,
resource_dict["cores"])- Documentation examples
- Internal parameter extraction from the resource_dict
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining direct resource parameter usage rg -l "cores_per_worker|num_cores|cores=|cwd=" --type py # Search for resource_dict usage to compare rg -l "resource_dict" --type pyLength of output: 1333
Script:
#!/bin/bash # Check actual usage of cores/cwd parameters in key files rg "cores_per_worker|num_cores|cores=|cwd=" -B 2 -A 2 executorlib/interactive/shared.py executorlib/cache/shared.py executorlib/standalone/cache/spawner.py # Check resource_dict implementation in the same files rg "resource_dict" -B 2 -A 2 executorlib/interactive/shared.py executorlib/cache/shared.py executorlib/standalone/cache/spawner.pyLength of output: 12916
| default_resource_dict = { | ||
| "cores": 1, | ||
| "cwd": None, | ||
| } | ||
| 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} | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying the resource dictionary initialization.
While the current implementation is correct, it could be more concise.
Consider this simpler implementation:
- default_resource_dict = {
- "cores": 1,
- "cwd": None,
- }
- 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}
- )
+ resource_dict = {
+ "cores": 1,
+ "cwd": None,
+ **(resource_dict or {})
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| default_resource_dict = { | |
| "cores": 1, | |
| "cwd": None, | |
| } | |
| 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} | |
| ) | |
| resource_dict = { | |
| "cores": 1, | |
| "cwd": None, | |
| **(resource_dict or {}) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (4)
executorlib/cache/executor.py (2)
18-22: Consider adding type hints for better maintainability.The PySQA integration with fallback is well-implemented. Consider adding type hints to make the function signature more explicit.
- execute_with_pysqa = execute_in_subprocess + execute_with_pysqa: callable = execute_in_subprocess
40-45: Enhance resource_dict documentation.The docstring should document all available resource_dict keys. Add descriptions for:
threads_per_coregpus_per_coreopenmpi_oversubscribeslurm_cmd_argsAlso, consider expanding the
pysqa_config_directorydescription to explain its purpose and format.executorlib/cache/shared.py (2)
53-55: Add type hints forresource_dictparameterThe type hint for
resource_dictparameter is missing. Consider adding it to maintain consistency with other parameters:- resource_dict: dict, + resource_dict: dict[str, Union[int, str, None]],
Line range hint
100-123: Consider implementing a ResourceManager classThe current implementation handles resources through dictionary operations spread across multiple locations. Consider encapsulating this logic in a dedicated
ResourceManagerclass to:
- Centralize resource validation and merging logic
- Provide type safety through a well-defined interface
- Make it easier to add new resource types in the future
- Reduce the risk of KeyError exceptions
Example implementation:
from dataclasses import dataclass from typing import Optional @dataclass class ExecutionResources: cores: int = 1 cwd: Optional[str] = None @classmethod def from_dict(cls, resource_dict: dict) -> 'ExecutionResources': return cls( cores=resource_dict.get('cores', 1), cwd=resource_dict.get('cwd') ) def merge(self, other: 'ExecutionResources') -> 'ExecutionResources': return ExecutionResources( cores=self.cores if self.cores != 1 else other.cores, cwd=self.cwd if self.cwd is not None else other.cwd )This would allow you to replace the dictionary operations with:
task_resources = ExecutionResources.from_dict(task_dict.get('resource_dict', {})) default_resources = ExecutionResources.from_dict(resource_dict) final_resources = task_resources.merge(default_resources)🧰 Tools
🪛 Ruff
110-110: Use
key not in dictinstead ofkey not in dict.keys()Remove
.keys()(SIM118)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- executorlib/init.py (7 hunks)
- executorlib/cache/executor.py (2 hunks)
- executorlib/cache/shared.py (4 hunks)
- tests/test_cache_executor_pysqa_flux.py (2 hunks)
- tests/test_cache_executor_serial.py (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_cache_executor_pysqa_flux.py
🔇 Additional comments (14)
executorlib/cache/executor.py (4)
10-15: LGTM! Well-organized imports for input validation.The new input validation imports are properly structured and support the enhanced resource validation functionality.
28-32: LGTM! Well-structured constructor signature changes.The parameter changes align with the PR objectives and follow Python conventions.
82-87: LGTM! Consistent resource management implementation.The process setup correctly uses the new resource_dict and PySQA configuration parameters.
49-71: Consider preserving validated resource keys.Currently, several keys are deleted after validation (
threads_per_core,gpus_per_core, etc.). This could be confusing as these values might be needed later. Consider either:
- Keeping these values in the dictionary for reference
- Moving validation-only keys to a separate validation function
Let's verify the validation function implementations:
tests/test_cache_executor_serial.py (3)
7-10: LGTM! Import organization follows best practices.The imports are properly organized and explicitly specify the required functions.
17-17: LGTM! Skip flag rename improves clarity.The rename from
skip_h5io_testtoskip_h5py_testbetter reflects its purpose of checking h5py library availability.Also applies to: 19-19, 31-31
35-35: Verify resource configuration completeness.While the FileExecutor instantiation has been updated to use resource_dict, please verify if all required resources are properly configured. The current instantiations have varying levels of resource configuration:
- Line 35: No resource configuration
- Line 42: No resource configuration
- Line 51-53: Only cwd configured
Also applies to: 42-42, 51-53
executorlib/__init__.py (7)
5-7: Approved: Import of_check_executorfunctionThe code correctly imports
_check_executorfromexecutorlib.standalone.inputcheck.
8-10: Approved: Import of_check_nested_flux_executorfunctionThe code correctly imports
_check_nested_flux_executorfromexecutorlib.standalone.inputcheck.
105-105: Approved: Addedpysqa_config_directoryparameter to__init__methodThe
pysqa_config_directoryparameter is correctly added to the__init__method signature, enhancing configurability for thepysqabackend.
107-107: Note the change of defaultblock_allocationvalue fromTruetoFalseThe default value of
block_allocationhas been changed toFalse. This may affect users who rely on the previous default behavior whereblock_allocationwasTrue.Action: Ensure this change is clearly documented in release notes or documentation to inform users about the potential impact.
126-126: Approved: Addedpysqa_config_directoryparameter to__new__methodThe
pysqa_config_directoryparameter is correctly added to the__new__method signature for consistent initialization.
128-128: Note the change of defaultblock_allocationvalue fromTruetoFalsein__new__methodThe default value of
block_allocationin the__new__method has been changed toFalse. This may impact existing code that depends on the previous default.Action: Ensure this change is properly communicated to users to prevent unexpected behavior.
209-231: Approved: Enhanced error handling forpysqabackend configurationsThe added error checks provide clear feedback for unsupported configurations when using the
pysqabackend, improving robustness and user experience.
tests/test_cache_executor_serial.py
Outdated
| "resource_dict": { | ||
| "cores": 1, | ||
| "threads_per_core": 1, | ||
| "gpus_per_core": 0, | ||
| "cwd": None, | ||
| "openmpi_oversubscribe": False, | ||
| "slurm_cmd_args": [], | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the resource dictionary configuration to reduce duplication.
The same detailed resource configuration is repeated across multiple test methods. This is an extension of the previously identified duplication issue, now with more complex configuration.
Consider creating a class-level helper method to generate resource dictionaries with optional overrides:
@classmethod
def get_resource_dict(cls, **overrides):
default_resources = {
"cores": 1,
"threads_per_core": 1,
"gpus_per_core": 0,
"cwd": None,
"openmpi_oversubscribe": False,
"slurm_cmd_args": [],
}
return {**default_resources, **overrides}Then use it in test methods:
resource_dict=self.get_resource_dict() # for default config
resource_dict=self.get_resource_dict(cwd=custom_cwd) # for custom valuesAlso applies to: 125-132, 173-180
executorlib/cache/shared.py
Outdated
| resource_dict=task_resource_dict, | ||
| config_directory=pysqa_config_directory, |
There was a problem hiding this comment.
💡 Codebase verification
Parameter naming inconsistency found in multiple files
The codebase shows inconsistent naming between pysqa_config_directory and config_directory. Here are the locations that need attention:
executorlib/cache/shared.py: Uses bothpysqa_config_directoryin the function signature butconfig_directoryin the execute_function callexecutorlib/cache/executor.py: Uses both naming conventions (pysqa_config_directoryin signature,config_directoryin dictionary)executorlib/standalone/cache/queue.pyandexecutorlib/standalone/cache/spawner.py: Still use the oldconfig_directorynaming
The parameter should be consistently named as pysqa_config_directory across all files since it specifically refers to the pysqa configuration directory.
🔗 Analysis chain
Ensure consistent parameter naming across the codebase
The parameter config_directory has been renamed to pysqa_config_directory to better reflect its purpose. Verify that this renaming is consistently applied across all related files and documentation.
Let me check the actual usage of config_directory in these files to determine if they need to be updated for consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of 'config_directory' that might need updating
rg -l 'config_directory' --type py
Length of output: 194
Script:
#!/bin/bash
# Check the context of config_directory usage in each file
for file in executorlib/__init__.py executorlib/cache/shared.py executorlib/cache/executor.py executorlib/standalone/cache/queue.py executorlib/standalone/cache/spawner.py; do
echo "=== $file ==="
rg 'config_directory' -B 2 -A 2 "$file"
done
Length of output: 6346
executorlib/__init__.py
Outdated
| @@ -47,6 +53,7 @@ class Executor: | |||
| flux_executor (flux.job.FluxExecutor): Flux Python interface to submit the workers to flux | |||
| flux_executor_pmi_mode (str): PMI interface to use (OpenMPI v5 requires pmix) default is None (Flux only) | |||
| flux_executor_nesting (bool): Provide hierarchically nested Flux job scheduler inside the submitted function. | |||
| pysqa_config_directory (str, optional): path to the pysqa config directory (only for pysqa based backend). | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use bool instead of boolean in docstrings for consistency
In the docstring, the parameter hostname_localhost is documented with the type boolean. For consistency with Python's type annotations and clarity, consider changing the type to bool.
Update block_allocation default value in docstring
The default value of block_allocation has changed from True to False in the code, but the docstring does not reflect this change. Please update the docstring to indicate the new default value.
executorlib/__init__.py
Outdated
| @@ -162,6 +171,7 @@ | |||
| of the individual function. | |||
| init_function (None): optional function to preset arguments for functions which are submitted later | |||
| disable_dependencies (boolean): Disable resolving future objects during the submission. | |||
| pysqa_config_directory (str, optional): path to the pysqa config directory (only for pysqa based backend). | |||
There was a problem hiding this comment.
Add pysqa_config_directory to the __new__ method's docstring
The pysqa_config_directory parameter is not documented in the __new__ method's docstring. Please add it to the Args section to maintain comprehensive documentation.
executorlib/__init__.py
Outdated
| elif "pysqa_" in backend and not plot_dependency_graph: | ||
| if cache_directory is None: | ||
| cache_directory = "executorlib_cache" | ||
| if max_workers != 1: | ||
| raise ValueError( | ||
| "The number of workers cannot be controlled with the pysqa based backend." | ||
| ) | ||
| if max_cores != 1: | ||
| raise ValueError( | ||
| "The number of cores cannot be controlled with the pysqa based backend." | ||
| ) | ||
| if hostname_localhost is not None: | ||
| raise ValueError( | ||
| "The option to connect to hosts based on their hostname is not available with the pysqa based backend." | ||
| ) | ||
| if block_allocation: | ||
| raise ValueError( | ||
| "The option block_allocation is not available with the pysqa based backend." | ||
| ) | ||
| if init_function is not None: | ||
| raise ValueError( | ||
| "The option to specify an init_function is not available with the pysqa based backend." | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine error messages for clarity and guidance
Some error messages could be more informative to help users troubleshoot issues effectively. Consider providing explanations or guidance on correct usage.
Example Revision:
-
Original message:
"The number of cores cannot be controlled with the pysqa based backend." -
Revised message:
"The 'pysqa' backend manages cores automatically; specifying 'max_cores' is not supported. Please remove 'max_cores' from your configuration when using 'pysqa'."
| - cores (int): number of MPI cores to be used for each function call | ||
| - threads_per_core (int): number of OpenMP threads to be used for each function call | ||
| - gpus_per_worker (int): number of GPUs per worker - defaults to 0 | ||
| - gpus_per_core (int): number of GPUs per worker - defaults to 0 |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can generate the final response as there are clear inconsistencies found in the codebase.
Documentation and code still contain references to old resource_dict keys
Several files still use the old keys and need to be updated:
notebooks/examples.ipynb: Contains multiple references tocores_per_workerandgpus_per_workerin documentation and code examplesREADME.md: Usescores_per_workerin documentationexecutorlib/__init__.py: Still documents the parameter ascores_per_workerandgpus_per_workerin docstringsexecutorlib/standalone/inputcheck.py: Contains function and error messages usinggpus_per_workertests/test_shared_input_check.py: Test cases usegpus_per_worker
While some files like executorlib/interactive/executor.py have been updated to use the new cores and gpus_per_core keys, there are still many places where the old terminology is used, which could lead to confusion.
🔗 Analysis chain
Ensure all references to old resource_dict keys are updated
The keys in resource_dict have been updated from cores_per_worker to cores and from gpus_per_worker to gpus_per_core. Please verify that all references to the old keys have been updated throughout the codebase and documentation to prevent inconsistencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for occurrences of deprecated resource_dict keys
# Searching for 'cores_per_worker'
rg 'cores_per_worker'
# Searching for 'gpus_per_worker'
rg 'gpus_per_worker'
Length of output: 3352
jan-janssen
force-pushed
the
file_executor_resource_dict
branch
from
2507bf6 to
69f2244
Compare
Summary by CodeRabbit
Release Notes
New Features
ExecutorandFileExecutorclasses, allowing users to specify resources using a structuredresource_dict.Executorclass.Bug Fixes
Tests
resource_dictformat.