Use resource dict for internal communication #455
Conversation
WalkthroughThe pull request introduces significant modifications to 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: 3
🧹 Outside diff range and nitpick comments (2)
tests/test_dependencies_executor.py (1)
102-107: Consider adding test cases for resource_dict edge cases.The test could be enhanced by adding cases that verify:
- Empty resource_dict
- Partial resource specifications
- Invalid resource values
Example test case:
def test_resource_dict_validation(self): with self.assertRaises(ValueError): create_executor( max_workers=1, max_cores=2, resource_dict={ "cores_per_worker": 0, # Invalid value "threads_per_core": -1, # Invalid value }, backend="local" )executorlib/__init__.py (1)
Line range hint
20-214: Document API changes and provide migration guide.The consolidation of resource parameters into a dictionary represents a significant change to the public API. While this improves internal communication, it may affect existing code that uses the Executor class.
Consider the following recommendations:
- Update the class docstring to reflect the new resource_dict parameter structure
- Add a migration guide in the documentation to help users transition from individual parameters to the new dictionary-based approach
- Consider adding a deprecation warning when individual parameters are used directly
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- executorlib/init.py (2 hunks)
- executorlib/interactive/executor.py (3 hunks)
- tests/test_dependencies_executor.py (1 hunks)
🔇 Additional comments (2)
executorlib/__init__.py (2)
Line range hint
200-214: Verify create_executor implementation.The resource_dict parameter is now passed to create_executor. Please ensure that the create_executor function has been properly updated to handle this new parameter structure.
#!/bin/bash # Description: Verify create_executor implementation # Test: Check if create_executor function has been updated to handle resource_dict # Search for create_executor function definition and its usage of resource_dict ast-grep --pattern 'def create_executor($$$) { $$$ }' # Search for any references to the old individual resource parameters rg 'def create_executor' -A 10 --type python
Line range hint
183-199: Verify ExecutorWithDependencies implementation.The resource_dict parameter is now passed to ExecutorWithDependencies. Please ensure that the ExecutorWithDependencies class has been properly updated to handle this new parameter structure.
✅ Verification successful
Based on the search results, I can now generate the final response:
ExecutorWithDependencies implementation has been properly updated to handle resource_dict.
The search results show that the ExecutorWithDependencies class inherits from ExecutorBase and uses the resource_dict parameter correctly. The resource dictionary is passed through the inheritance chain and properly handled in the executor implementation. The code shows consistent usage of resource_dict throughout the codebase, including:
- Proper initialization in the base executor classes
- Consistent parameter passing in the dependency executor
- Resource dictionary validation and processing
- Correct mapping of old resource parameters to the new resource_dict structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify ExecutorWithDependencies implementation # Test: Check if ExecutorWithDependencies class has been updated to handle resource_dict # Search for ExecutorWithDependencies class definition and its usage of resource_dict ast-grep --pattern 'class ExecutorWithDependencies { $$$ def __init__($$$) { $$$ } $$$ }' # Search for any references to the old individual resource parameters rg -A 5 'cores_per_worker|threads_per_core|gpus_per_worker|cwd|openmpi_oversubscribe|slurm_cmd_args' --type pythonLength of output: 377
Script:
#!/bin/bash # Let's try a different approach to find the ExecutorWithDependencies class and its usage of resource_dict # Search for ExecutorWithDependencies class definition rg "class ExecutorWithDependencies" -A 20 # Search for resource_dict in the codebase rg "resource_dict" -A 5 # Search for the old resource parameters without file type restriction rg -A 5 'cores_per_worker|threads_per_core|gpus_per_worker|cwd|openmpi_oversubscribe|slurm_cmd_args'Length of output: 107298
tests/test_dependencies_executor.py
Outdated
| resource_dict={ | ||
| "cores_per_worker": 1, | ||
| "threads_per_core": 1, | ||
| "gpus_per_worker": 0, | ||
| "openmpi_oversubscribe": False, | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Migration to resource_dict is incomplete across test files
The codebase shows inconsistent usage of resource parameters. While the reviewed test file correctly uses the consolidated resource_dict, many other test files are still using individual parameters (cores_per_worker, threads_per_core, etc.) directly. This indicates that the migration to resource_dict is not complete.
Key files needing attention:
tests/test_flux_executor.py: Uses direct parameter passingtests/test_local_executor.py: Mixed usage of old and new patternstests/test_shell_executor.py: Still using individual parameters- Multiple test files using execute_parallel_tasks with individual parameters
🔗 Analysis chain
LGTM! Consider enhancing resource allocation verification.
The resource_dict implementation correctly consolidates the resource parameters. The test maintains the existing functionality while adapting to the new API.
Let's verify if other tests in the codebase are consistently using the new resource_dict pattern:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct usage of the old parameter names
rg -l "cores_per_worker|threads_per_core|gpus_per_worker|openmpi_oversubscribe" "tests/"
Length of output: 562
Script:
#!/bin/bash
# Let's check the actual usage patterns in these files to determine if they're using resource_dict
rg -A 3 -B 3 "cores_per_worker|threads_per_core|gpus_per_worker|openmpi_oversubscribe" "tests/" --no-filename
Length of output: 9537
executorlib/__init__.py
Outdated
| resource_dict = { | ||
| "cores": cores_per_worker, | ||
| "threads_per_core": threads_per_core, | ||
| "gpus_per_core": int(gpus_per_worker / cores_per_worker), | ||
| "cwd": cwd, | ||
| "openmpi_oversubscribe": openmpi_oversubscribe, | ||
| "slurm_cmd_args": slurm_cmd_args, | ||
| } |
There was a problem hiding this comment.
Validate GPU allocation and improve naming consistency.
The resource dictionary creation has potential issues:
- The GPU division could lead to unexpected results if gpus_per_worker is not evenly divisible by cores_per_worker
- Missing validation for cores_per_worker being non-zero
- Inconsistent parameter naming between input and dictionary ("cores" vs "cores_per_worker")
Consider applying these improvements:
resource_dict = {
- "cores": cores_per_worker,
+ "cores_per_worker": cores_per_worker,
"threads_per_core": threads_per_core,
- "gpus_per_core": int(gpus_per_worker / cores_per_worker),
+ "gpus_per_core": (gpus_per_worker // cores_per_worker) if cores_per_worker > 0 else 0,
"cwd": cwd,
"openmpi_oversubscribe": openmpi_oversubscribe,
"slurm_cmd_args": slurm_cmd_args,
}
+if cores_per_worker > 0 and gpus_per_worker % cores_per_worker != 0:
+ raise ValueError(f"gpus_per_worker ({gpus_per_worker}) must be evenly divisible by cores_per_worker ({cores_per_worker})")📝 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 = { | |
| "cores": cores_per_worker, | |
| "threads_per_core": threads_per_core, | |
| "gpus_per_core": int(gpus_per_worker / cores_per_worker), | |
| "cwd": cwd, | |
| "openmpi_oversubscribe": openmpi_oversubscribe, | |
| "slurm_cmd_args": slurm_cmd_args, | |
| } | |
| resource_dict = { | |
| "cores_per_worker": cores_per_worker, | |
| "threads_per_core": threads_per_core, | |
| "gpus_per_core": (gpus_per_worker // cores_per_worker) if cores_per_worker > 0 else 0, | |
| "cwd": cwd, | |
| "openmpi_oversubscribe": openmpi_oversubscribe, | |
| "slurm_cmd_args": slurm_cmd_args, | |
| } | |
| if cores_per_worker > 0 and gpus_per_worker % cores_per_worker != 0: | |
| raise ValueError(f"gpus_per_worker ({gpus_per_worker}) must be evenly divisible by cores_per_worker ({cores_per_worker})") |
| resource_dict (dict): A dictionary of resources required by the task. With the following keys: | ||
| - cores_per_worker (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 | ||
| - cwd (str/None): current working directory where the parallel python task is executed | ||
| - openmpi_oversubscribe (bool): adds the `--oversubscribe` command line flag (OpenMPI and | ||
| SLURM only) - default False | ||
| - slurm_cmd_args (list): Additional command line arguments for the srun call (SLURM only) |
There was a problem hiding this comment.
Inconsistent key names in resource_dict
The keys used in resource_dict are inconsistent between the documentation, default values, and usage in the code:
- In the docstring (lines 177-184), keys like
cores_per_workerandgpus_per_workerare documented. - In
default_resource_dict(lines 205-211), the keys arecoresandgpus_per_core. - In usage (lines 218-219, 263-264), keys like
cores_per_workerandgpus_per_workerare expected.
This inconsistency can lead to confusion and potential bugs since the keys may not match as intended.
To resolve this inconsistency, align the key names across the docstring, default values, and code usage. Update default_resource_dict and related code to use consistent key names:
default_resource_dict = {
- "cores": 1,
+ "cores_per_worker": 1,
"threads_per_core": 1,
- "gpus_per_core": 0,
+ "gpus_per_worker": 0,
"cwd": None,
"openmpi_oversubscribe": False,
"slurm_cmd_args": [],
}Update the usage in the code accordingly:
-cores_per_worker = resource_dict["cores"]
+cores_per_worker = resource_dict["cores_per_worker"]
...
-check_gpus_per_worker(gpus_per_worker=resource_dict["gpus_per_worker"])
+check_gpus_per_worker(gpus_per_worker=resource_dict["gpus_per_worker"])Ensure that all references to these keys are updated for consistency.
Also applies to: 205-211, 218-219, 263-264
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (10)
tests/test_executor_backend_mpi_noblock.py (2)
Line range hint
8-10: Rename the helper function to avoid parameter shadowing.The helper function
resource_dicthas the same name as its parameter, which could lead to confusion and potential issues. Consider renaming it to be more descriptive of its purpose.-def resource_dict(resource_dict): +def validate_resource_dict(resource_dict): return resource_dict
Line range hint
42-77: Enhance test coverage for resource dictionary validation.The current tests only cover basic error cases. Consider adding tests for:
- Invalid resource values (negative or zero)
- Missing required keys in the resource_dict
- Conflicting resource specifications
- Edge cases with maximum allowed values
Would you like me to help generate additional test cases to improve coverage?
tests/test_executor_backend_flux.py (1)
Line range hint
63-106: Consider adding documentation for resource_dict structureWhile the migration to resource_dict is implemented consistently across all test cases, it would be beneficial to add documentation that describes:
- The available keys in the resource_dict
- The expected value types and ranges
- The relationship between different resource specifications (if any)
This would help users understand the new API better and ensure correct usage.
tests/test_executor_backend_mpi.py (1)
74-82: Consider adding more comprehensive resource dictionary test cases.The test suite would benefit from additional test cases covering:
- Empty resource dictionary
- Resource dictionary with unknown keys
- Resource dictionary with invalid value types
- Resource dictionary with negative or zero values
- Complex combinations of resource parameters
This would help ensure robust validation of the new resource management system.
Would you like me to help generate these additional test cases?
README.md (2)
46-46: Add migration guidance for the new resource_dict parameter.The change from individual parameters to a consolidated resource_dict represents a significant API change. Consider adding a section that:
- Explains the motivation behind using resource_dict
- Lists all supported resource types
- Provides migration examples from old to new format
Example addition:
### Resource Dictionary The `resource_dict` parameter provides a flexible way to specify various computing resources: - `cores`: Number of CPU cores per worker - `threads`: Number of threads per core - `gpus`: Number of GPUs per worker Previous API: ```python with Executor(cores_per_worker=2, threads_per_core=4) as exe: ...New API:
with Executor(resource_dict={"cores": 2, "threads": 4}) as exe: ...--- `46-46`: **Consider expanding the example to showcase resource_dict capabilities.** The current example only demonstrates setting cores. Consider expanding it to show: 1. Multiple resource types (cores, threads, GPUs) 2. Resource combinations 3. Common use cases Example addition: ```python # Example with multiple resource types resource_dict = { "cores": 2, "threads": 4, "gpus": 1 } with Executor(max_cores=2, executor=flux_exe, resource_dict=resource_dict) as exe: fs = exe.submit(calc, 3) print(fs.result())executorlib/__init__.py (1)
39-46: Enhance resource_dict documentation with defaults and update example.While the documentation clearly lists all available keys, it would be helpful to:
- Document the default values for each parameter
- Update the example in the class docstring to demonstrate the new
resource_dictusage patternConsider updating the example to:
>>> with Executor(resource_dict={ >>> "cores_per_worker": 2, >>> "threads_per_core": 1 >>> }, init_function=init_k) as p: >>> fs = p.submit(calc, 2, j=4) >>> print(fs.result())notebooks/examples.ipynb (3)
216-223: Consider enhancing resource dictionary documentation with type hints.The resource dictionary implementation effectively consolidates resource parameters. Consider adding type hints in the documentation for each parameter to make it clearer what values are expected.
Example documentation format:
resource_dict = { "cores": int, # Number of CPU cores "threads_per_core": int, # Number of threads per CPU core "gpus_per_core": int, # Number of GPUs per core "cwd": str, # Working directory path "openmpi_oversubscribe": bool, # Enable/disable OpenMPI oversubscription "slurm_cmd_args": list, # Additional SLURM command arguments }
476-482: Consider adding more comprehensive resource configuration examples.While the current example effectively demonstrates basic resource configuration, consider adding examples that show:
- Combined CPU and GPU configurations
- Different thread-per-core scenarios
- Resource oversubscription cases
Example additions:
# CPU + GPU configuration resource_dict={ "cores": 2, "gpus_per_core": 1, "threads_per_core": 2 } # Oversubscription example resource_dict={ "cores": 4, "threads_per_core": 2, "openmpi_oversubscribe": True }
Line range hint
1-763: Consider adding a troubleshooting section to the documentation.The documentation is comprehensive and well-structured. Consider adding a troubleshooting section that covers:
- Common error scenarios and their solutions
- Resource configuration pitfalls
- Platform-specific issues (Linux, MacOS, Windows)
- Performance optimization tips
This would help users quickly resolve common issues and optimize their resource usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- README.md (1 hunks)
- executorlib/init.py (6 hunks)
- executorlib/interactive/executor.py (3 hunks)
- notebooks/examples.ipynb (26 hunks)
- tests/test_dependencies_executor.py (1 hunks)
- tests/test_executor_backend_flux.py (4 hunks)
- tests/test_executor_backend_mpi.py (3 hunks)
- tests/test_executor_backend_mpi_noblock.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_dependencies_executor.py
🔇 Additional comments (9)
tests/test_executor_backend_mpi_noblock.py (1)
48-51: LGTM! The resource dictionary implementation maintains error handling.The test cases correctly verify TypeError exceptions for invalid resource configurations using the new resource_dict parameter structure.
Let's verify the error handling implementation in the Executor class:
Also applies to: 57-60
✅ Verification successful
These scripts will help us:
- Find the Executor class implementation
- Understand how resource_dict is used across the codebase
- Locate all test files related to Executor to verify test coverage
Based on the search results, I can now provide a final response. The codebase shows comprehensive implementation and testing of the resource_dict parameter:
The resource dictionary implementation is correctly tested and implemented
The test cases in
test_executor_backend_mpi_noblock.pyare part of a well-structured test suite that verifies:
- Resource dictionary validation in
executorlib/standalone/inputcheck.py- Default values and parameter merging in
executorlib/__init__.py- Error handling across different executor backends (MPI, Flux, SLURM)
- Consistent resource specification format across the codebase
The test coverage is adequate with multiple test files verifying different aspects of resource dictionary handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the resource_dict validation in Executor implementation ast-grep --pattern 'class Executor { $$$ def __new__($$$) { $$$ resource_dict $$$ } }'Length of output: 106
Script:
#!/bin/bash # Let's try a broader search for Executor class and resource_dict usage rg -A 5 "class Executor"Length of output: 1898
Script:
#!/bin/bash # Search for resource_dict usage in Python files rg "resource_dict" --type py -A 3Length of output: 30868
Script:
#!/bin/bash # Search for test files related to Executor fd "test.*executor.*\.py$"Length of output: 443
tests/test_executor_backend_flux.py (4)
63-63: LGTM: Correct migration to resource_dict formatThe threads_per_core parameter has been properly migrated to the new resource_dict format while maintaining the same functionality.
91-91: Same resource_dict pattern as previous test
106-106: Same resource_dict pattern as previous tests
78-78: Verify the resource key naming conventionThe parameter has changed from
cores_per_workertocoresin the resource_dict. Please verify if this naming change is intentional and consistent with the new API design.✅ Verification successful
API change from
cores_per_workertocoresis intentional and consistentThe codebase shows a clear and consistent transition from
cores_per_workertocoresin the resource dictionary. This is evidenced by:
- The interactive executor explicitly maps
cores_per_workertocoresinexecutorlib/interactive/executor.py:cores_per_worker = resource_dict["cores"]- All recent test files use
resource_dict={"cores": N}format- The cache executor provides backward compatibility by mapping
cores_per_workertocoresinexecutorlib/cache/shared.py- Documentation and examples consistently use the new
coreskey in resource dictionaries🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of cores_per_worker and cores in resource specifications echo "Searching for cores_per_worker usage:" rg "cores_per_worker" -A 2 -B 2 echo -e "\nSearching for resource_dict with cores key:" rg 'resource_dict.*cores' -A 2 -B 2Length of output: 20809
tests/test_executor_backend_mpi.py (2)
95-95: LGTM! Resource configuration and cache testing look good.The migration to
resource_dictmaintains the same test coverage while properly validating both the MPI functionality and caching behavior.
74-82: Enhance error case testing for resource validation.While the test verifies that TypeError is raised, it could be more comprehensive:
- The test doesn't verify the error messages
- It's not clear why these resource configurations are invalid
- The validation rules for the resource dictionary aren't documented
Let's check if there's documentation or validation logic for the resource dictionary:
#!/bin/bash # Search for resource dictionary validation logic rg -A 5 "resource_dict.*validate|validate.*resource_dict" # Search for resource dictionary documentation fd "README|CONTRIBUTING" | xargs rg -l "resource_dict"executorlib/__init__.py (1)
189-189: LGTM: Consistent resource_dict usage.The
resource_dictis correctly passed to both executor creation paths.Also applies to: 207-207
executorlib/interactive/executor.py (1)
154-154: Introduction ofresource_dictenhances resource managementThe addition of the
resource_dictparameter consolidates resource-related arguments, simplifying the function signature and improving flexibility.
| @@ -58,7 +58,7 @@ def test_meta_executor_single(self): | |||
| def test_meta_executor_parallel(self): | |||
| with Executor( | |||
| max_workers=2, | |||
| cores_per_worker=2, | |||
| resource_dict={"cores": 2}, | |||
There was a problem hiding this comment.
💡 Codebase verification
Migration to resource_dict is incomplete - several files still use cores_per_worker
The search results reveal that several test files and implementation files still use the old cores_per_worker parameter instead of the new resource_dict approach:
tests/test_cache_executor_serial.pytests/test_cache_executor_pysqa_flux.pytests/test_cache_executor_mpi.py
The migration needs to be completed consistently across all test files to match the new resource management approach.
🔗 Analysis chain
LGTM! Resource dictionary implementation looks correct.
The migration from cores_per_worker=2 to resource_dict={"cores": 2} maintains the same test coverage while aligning with the new resource management approach.
Let's verify that all test files have been updated to use the new resource_dict:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of cores_per_worker to ensure complete migration
rg "cores_per_worker" "tests/"
Length of output: 124
Script:
#!/bin/bash
# Let's search for both old and new patterns to understand the migration status
echo "=== Searching for cores_per_worker usage ==="
rg "cores_per_worker" -A 2
echo -e "\n=== Searching for resource_dict usage in tests ==="
rg "resource_dict" "tests/" -A 2
Length of output: 13827
| @@ -43,7 +43,7 @@ def calc(i): | |||
| return i, size, rank | |||
|
|
|||
| with flux.job.FluxExecutor() as flux_exe: | |||
| with Executor(max_cores=2, cores_per_worker=2, executor=flux_exe) as exe: | |||
| with Executor(max_cores=2, executor=flux_exe, resource_dict={"cores": 2}) as exe: | |||
There was a problem hiding this comment.
Update the explanatory text to match the new API.
The example code has been updated to use resource_dict={"cores": 2}, but the explanatory text below still references the old cores_per_worker=2 parameter. This inconsistency could confuse users.
Consider updating the paragraph that starts with "The interface of the standard..." to reflect the new API:
-The interface of the standard [concurrent.futures.Executor](https://docs.python.org/3/library/concurrent.futures.html#module-concurrent.futures)
-is extended by adding the option `cores_per_worker=2` to assign multiple MPI ranks to each function call.
+The interface of the standard [concurrent.futures.Executor](https://docs.python.org/3/library/concurrent.futures.html#module-concurrent.futures)
+is extended by adding the option `resource_dict={"cores": 2}` to assign multiple MPI ranks to each function call.📝 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.
| with Executor(max_cores=2, executor=flux_exe, resource_dict={"cores": 2}) as exe: | |
| The interface of the standard [concurrent.futures.Executor](https://docs.python.org/3/library/concurrent.futures.html#module-concurrent.futures) | |
| is extended by adding the option `resource_dict={"cores": 2}` to assign multiple MPI ranks to each function call. |
| 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.
Add validation for resource_dict values.
The code accepts arbitrary values without validation, which could lead to resource exhaustion or other security issues. Consider adding validation for:
- Negative values for cores, threads, and GPUs
- Unreasonably large values that could impact system stability
- Type checking for numeric parameters
Example implementation:
def validate_resource_dict(resource_dict):
for key in ['cores_per_worker', 'threads_per_core', 'gpus_per_worker']:
if key in resource_dict:
value = resource_dict[key]
if not isinstance(value, int):
raise TypeError(f"{key} must be an integer")
if value < 0:
raise ValueError(f"{key} cannot be negative")
if value > MAX_ALLOWED_VALUE: # Define appropriate limits
raise ValueError(f"{key} exceeds maximum allowed value")Would you like me to provide a complete implementation with appropriate resource limits?
| 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} | ||
| ) |
There was a problem hiding this comment.
Fix inconsistent parameter naming in default_resource_dict.
The keys in default_resource_dict don't match the documented parameter names:
- "cores" vs documented "cores_per_worker"
- "gpus_per_core" vs documented "gpus_per_worker"
Apply this fix:
default_resource_dict = {
- "cores": 1,
- "gpus_per_core": 0,
+ "cores_per_worker": 1,
+ "gpus_per_worker": 0,
"threads_per_core": 1,
"cwd": None,
"openmpi_oversubscribe": False,
"slurm_cmd_args": [],
}📝 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, | |
| "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} | |
| ) | |
| default_resource_dict = { | |
| "cores_per_worker": 1, | |
| "threads_per_core": 1, | |
| "gpus_per_worker": 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} | |
| ) |
| check_executor(executor=flux_executor) | ||
| check_nested_flux_executor(nested_flux_executor=flux_executor_nesting) | ||
| executor_kwargs["openmpi_oversubscribe"] = openmpi_oversubscribe | ||
| check_threads_per_core(threads_per_core=resource_dict["threads_per_core"]) | ||
| check_gpus_per_worker(gpus_per_worker=resource_dict["gpus_per_core"]) |
There was a problem hiding this comment.
Incorrect key used for gpus_per_worker in local backend
In the call to check_gpus_per_worker, gpus_per_worker=resource_dict["gpus_per_core"] is passed. However, the key should be gpus_per_worker to align with the expected parameter.
Apply this diff to correct the key:
-check_gpus_per_worker(gpus_per_worker=resource_dict["gpus_per_core"])
+check_gpus_per_worker(gpus_per_worker=resource_dict["gpus_per_worker"])📝 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.
| check_gpus_per_worker(gpus_per_worker=resource_dict["gpus_per_core"]) | |
| check_gpus_per_worker(gpus_per_worker=resource_dict["gpus_per_worker"]) |
Summary by CodeRabbit
New Features
resource_dictfor executor creation.Bug Fixes
Tests
Documentation