Update validate_number_of_cores() #488
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request primarily enhance the logic for determining 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 (
|
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
tests/test_shared_input_check.py (1)
109-117: Consider adding more test cases for comprehensive coverage.While the current test cases cover basic scenarios, consider adding tests for:
- All parameters being non-None (e.g., max_cores=2, max_workers=2, cores_per_worker=1)
- Edge cases with cores_per_worker (e.g., cores_per_worker > max_cores)
Example additions:
def test_validate_number_of_cores(self): # Existing tests... # Test all parameters specified self.assertIsInstance( validate_number_of_cores(max_cores=2, max_workers=2, cores_per_worker=1), int ) # Test invalid cores_per_worker with self.assertRaises(ValueError): validate_number_of_cores(max_cores=2, max_workers=2, cores_per_worker=3)executorlib/interactive/executor.py (1)
Line range hint
220-284: Well-structured centralization of worker validation logic.The changes successfully centralize the worker count validation across all backends while maintaining backend-specific requirements. This architectural improvement:
- Reduces code duplication
- Ensures consistent validation
- Maintains backend-specific configurations
Consider documenting the backend-specific validation differences (especially the
set_local_coresparameter) in the function docstring to help future maintainers understand the design decisions.executorlib/interactive/shared.py (1)
418-427: Reduce code duplication in resource management logic.The current implementation duplicates the dictionary comprehension for filtering completed tasks.
Consider this more DRY approach:
if max_cores is not None: - while sum(active_task_dict.values()) + cores_requested > max_cores: - active_task_dict = { - k: v for k, v in active_task_dict.items() if not k.done() - } - elif max_workers is not None and max_cores is None: - while len(active_task_dict.values()) + 1 > max_workers: - active_task_dict = { - k: v for k, v in active_task_dict.items() if not k.done() - } + limit = max_cores + current_usage = lambda d: sum(d.values()) + cores_requested + elif max_workers is not None: + limit = max_workers + current_usage = lambda d: len(d.values()) + 1 + else: + return active_task_dict + + while current_usage(active_task_dict) > limit: + active_task_dict = { + k: v for k, v in active_task_dict.items() if not k.done() + }executorlib/standalone/inputcheck.py (2)
173-190: Consider renaming the function for clarityThe function
validate_number_of_coresnow calculates and returnsmax_workers, which might cause confusion since the name suggests it validates cores. Renaming the function to better reflect its purpose could improve code readability, such ascalculate_max_workers.
189-189: Use floor division for integer calculationIn the line
max_workers = int(max_cores / cores_per_worker), consider using floor division//to directly obtain an integer result without type casting.Apply this minor change:
max_workers = int(max_cores / cores_per_worker) # Change to: max_workers = max_cores // cores_per_worker
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
executorlib/interactive/executor.py(3 hunks)executorlib/interactive/shared.py(9 hunks)executorlib/standalone/inputcheck.py(1 hunks)tests/test_shared_input_check.py(1 hunks)
🧰 Additional context used
🪛 Ruff
executorlib/interactive/shared.py
184-184: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🔇 Additional comments (8)
tests/test_shared_input_check.py (2)
101-104: LGTM: Well-structured test for invalid input combination.
The test case properly validates that the function raises ValueError when all parameters are None, which is a critical edge case to check.
105-108: Verify if TypeError is the expected exception for this input combination.
The test expects TypeError when max_cores=1 and other parameters are None. This seems counterintuitive as having a single core specified could be a valid configuration. Could you verify if this is the intended behavior?
✅ Verification successful
Let me gather more context about the implementation to understand the validation logic better.
Now I can provide the final response based on the implementation details:
TypeError is correctly expected when cores_per_worker is None
The implementation shows that when max_cores is provided but max_workers is None, the function attempts to calculate max_workers = int(max_cores / cores_per_worker). Since cores_per_worker is None in the test case, this division operation would raise a TypeError, making this test case valid and intentional.
Looking at the test cases:
- First case tests ValueError when both
max_coresandmax_workersare None - Second case tests TypeError when division by None is attempted
- Third case (visible in output) tests successful execution when both
max_coresandcores_per_workerare 1
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation to understand why TypeError is raised for this case
# Look for the validate_number_of_cores implementation
rg -A 10 "def validate_number_of_cores"
Length of output: 808
Script:
#!/bin/bash
# Get the full implementation of validate_number_of_cores
rg -A 30 "def validate_number_of_cores"
# Also search for any related test cases
rg -B 2 -A 5 "TypeError.*validate_number_of_cores" tests/
Length of output: 1542
Script:
#!/bin/bash
# Get more context around the test case
rg -B 10 -A 10 "def test_validate_number_of_cores" tests/
# Also check for any division operations in the implementation
rg -B 2 -A 2 "cores_per_worker" executorlib/
Length of output: 4633
executorlib/interactive/executor.py (3)
242-247: LGTM! Consistent implementation with flux backend.
The changes maintain consistency with the flux backend implementation, ensuring uniform validation across different execution environments.
Also applies to: 254-254
272-277: Verify the local cores setting impact.
The implementation correctly sets set_local_cores=True for the local backend, which differs from other backends. This suggests special handling for local execution.
Let's verify the local cores handling:
#!/bin/bash
# Description: Verify local cores handling in validate_number_of_cores
# Look for any special handling of local cores in the validation function
rg -A 5 "set_local_cores.*=.*True"
# Check for any system-specific core counting logic
ast-grep --pattern 'validate_number_of_cores($$$set_local_cores = True$$$)'Also applies to: 284-284
220-225: LGTM! Verify max_workers validation across all code paths.
The changes correctly implement the new validation logic for the flux backend, maintaining consistency between block and non-block allocation paths.
Let's verify the validation implementation:
Also applies to: 232-232
✅ Verification successful
Validation of max_workers is consistent and correct across all code paths
The verification shows that:
- All three usages of
validate_number_of_cores()in executor.py correctly pass the required parameters - The implementation in inputcheck.py properly handles all validation scenarios:
- When both max_cores and max_workers are None
- When max_cores is provided but max_workers is None
- The set_local_cores parameter correctly determines whether to use local CPU count
- No other functions are calculating max_workers independently
- Test coverage exists for the validation function
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify validate_number_of_cores usage consistency
# Look for any direct max_workers calculations that might bypass validation
rg -g '!*.pyc' -g '!*test*' "max_workers.*=.*max_cores.*cores_per_worker"
# Look for all validate_number_of_cores usages to ensure consistent parameters
ast-grep --pattern 'validate_number_of_cores($$$)'
Length of output: 506
Script:
#!/bin/bash
# Let's search for all validate_number_of_cores usages with better patterns
ast-grep --pattern 'validate_number_of_cores($$$)'
# Also search for raw text occurrences to catch any string references
rg -A 3 "validate_number_of_cores"
# Look for the actual function definition
ast-grep --pattern 'def validate_number_of_cores($$$)'
Length of output: 5500
Script:
#!/bin/bash
# Let's look at the actual implementation of validate_number_of_cores
cat executorlib/standalone/inputcheck.py
# And check if there are any other functions that might be calculating max_workers
rg -g '!*.pyc' -g '!*test*' "def.*max_workers"
Length of output: 6680
executorlib/interactive/shared.py (2)
Line range hint 261-275: LGTM! Clear documentation of parameter relationship.
The changes maintain backwards compatibility while providing clear guidance on preferring max_cores over max_workers.
Line range hint 514-530: LGTM! Consistent implementation of resource management.
The changes maintain consistency with other functions and provide clear documentation of the parameter relationship.
Also applies to: 553-553
executorlib/standalone/inputcheck.py (1)
181-187: Verify the use of local CPU count in resource allocation
When both max_cores and max_workers are None and set_local_cores is True, max_workers is set to multiprocessing.cpu_count(). In distributed environments, using the local CPU count may not align with the desired resource allocation strategy. Ensure this behavior is intended and consistent across different execution environments.
executorlib/interactive/shared.py
Outdated
| max_cores: Optional[int] = None, | ||
| max_workers: Optional[int] = None, | ||
| executor_kwargs: dict = {}, | ||
| spawner: BaseSpawner = MpiExecSpawner, | ||
| ): | ||
| super().__init__() | ||
| executor_kwargs["future_queue"] = self._future_queue | ||
| executor_kwargs["spawner"] = spawner | ||
| executor_kwargs["max_cores"] = max_cores | ||
| executor_kwargs["max_workers"] = max_workers |
There was a problem hiding this comment.
Fix mutable default argument.
The executor_kwargs parameter uses a mutable default argument which can lead to unexpected behavior if the same dictionary is shared across instances.
Apply this fix:
def __init__(
self,
max_cores: Optional[int] = None,
max_workers: Optional[int] = None,
- executor_kwargs: dict = {},
+ executor_kwargs: Optional[dict] = None,
):
super().__init__()
+ if executor_kwargs is None:
+ executor_kwargs = {}
executor_kwargs["future_queue"] = self._future_queueCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff
184-184: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
| cores_per_worker: Optional[int] = None, | ||
| set_local_cores: bool = False, | ||
| ) -> int: | ||
| """ | ||
| Validate the number of cores and return the appropriate value. | ||
| """ | ||
| if max_workers is None and max_cores is None: | ||
| return multiprocessing.cpu_count() | ||
| elif max_workers is not None and max_cores is None: | ||
| return max_workers | ||
| else: | ||
| return max_cores | ||
| if max_cores is None and max_workers is None: | ||
| if not set_local_cores: | ||
| raise ValueError( | ||
| "Block allocation requires a fixed set of computational resources. Neither max_cores nor max_workers are defined." | ||
| ) | ||
| else: | ||
| max_workers = multiprocessing.cpu_count() | ||
| elif max_cores is not None and max_workers is None: | ||
| max_workers = int(max_cores / cores_per_worker) |
There was a problem hiding this comment.
Prevent potential division by zero in validate_number_of_cores
In the calculation max_workers = int(max_cores / cores_per_worker), there is a risk of division by zero or a TypeError if cores_per_worker is None or zero. Ensure that cores_per_worker is validated to be a non-zero integer before performing the division.
Consider adding a check to validate cores_per_worker:
elif max_cores is not None and max_workers is None:
+ if cores_per_worker is None or cores_per_worker == 0:
+ raise ValueError("cores_per_worker must be a non-zero integer.")
max_workers = int(max_cores / 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.
| cores_per_worker: Optional[int] = None, | |
| set_local_cores: bool = False, | |
| ) -> int: | |
| """ | |
| Validate the number of cores and return the appropriate value. | |
| """ | |
| if max_workers is None and max_cores is None: | |
| return multiprocessing.cpu_count() | |
| elif max_workers is not None and max_cores is None: | |
| return max_workers | |
| else: | |
| return max_cores | |
| if max_cores is None and max_workers is None: | |
| if not set_local_cores: | |
| raise ValueError( | |
| "Block allocation requires a fixed set of computational resources. Neither max_cores nor max_workers are defined." | |
| ) | |
| else: | |
| max_workers = multiprocessing.cpu_count() | |
| elif max_cores is not None and max_workers is None: | |
| max_workers = int(max_cores / cores_per_worker) | |
| cores_per_worker: Optional[int] = None, | |
| set_local_cores: bool = False, | |
| ) -> int: | |
| """ | |
| Validate the number of cores and return the appropriate value. | |
| """ | |
| if max_cores is None and max_workers is None: | |
| if not set_local_cores: | |
| raise ValueError( | |
| "Block allocation requires a fixed set of computational resources. Neither max_cores nor max_workers are defined." | |
| ) | |
| else: | |
| max_workers = multiprocessing.cpu_count() | |
| elif max_cores is not None and max_workers is None: | |
| if cores_per_worker is None or cores_per_worker == 0: | |
| raise ValueError("cores_per_worker must be a non-zero integer.") | |
| max_workers = int(max_cores / cores_per_worker) |
|
With these changes the tests start to fail sporadically. So there is still something wrong. |
Summary by CodeRabbit
New Features
max_coresandmax_workersin theInteractiveStepExecutor.Bug Fixes
Tests
validate_number_of_coresfunction, ensuring proper exception handling and return types.