Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set max_cores and max_workers to None #478

Merged
merged 7 commits into from
Nov 6, 2024
Merged

Set max_cores and max_workers to None #478

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7aa0de2
Set max_cores and max_workers to None
jan-janssen Nov 6, 2024
f8f69f6
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 6, 2024
9739df8
Merge remote-tracking branch 'origin/main' into core_limit
jan-janssen Nov 6, 2024
3e824b0
By default get the number of cores from the multiprocessing package
jan-janssen Nov 6, 2024
10ffbf3
Test Fixes
jan-janssen Nov 6, 2024
c6c6b2e
more fixes
jan-janssen Nov 6, 2024
b5c0a14
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions executorlib/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@ class Executor:

def __init__(
self,
max_workers: int = 1,
max_workers: Optional[int] = None,
backend: str = "local",
cache_directory: Optional[str] = None,
max_cores: int = 1,
max_cores: Optional[int] = None,
resource_dict: Optional[dict] = None,
flux_executor=None,
flux_executor_pmi_mode: Optional[str] = None,
Expand All @@ -104,10 +104,10 @@ def __init__(

def __new__(
cls,
max_workers: int = 1,
max_workers: Optional[int] = None,
backend: str = "local",
cache_directory: Optional[str] = None,
max_cores: int = 1,
max_cores: Optional[int] = None,
resource_dict: Optional[dict] = None,
flux_executor=None,
flux_executor_pmi_mode: Optional[str] = None,
Expand Down
4 changes: 2 additions & 2 deletions executorlib/interactive/executor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,9 @@ def __exit__(


def create_executor(
max_workers: int = 1,
max_workers: Optional[int] = None,
backend: str = "local",
max_cores: int = 1,
max_cores: Optional[int] = None,
Comment on lines +150 to +152
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Type mismatch in validate_number_of_cores function signature

The function signature in executorlib/standalone/inputcheck.py doesn't match the optional types:

def validate_number_of_cores(max_cores: int, max_workers: int) -> int:

While the implementation correctly handles None values, the type hints need to be updated to Optional[int] to match the caller's signature.

  • executorlib/standalone/inputcheck.py: Update parameter types to Optional[int]
🔗 Analysis chain

Verify validation handling for None values.

While making max_workers and max_cores optional aligns with the PR objectives, we should ensure that validate_number_of_cores properly handles None values for both parameters.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for validate_number_of_cores implementation to verify None handling
ast-grep --pattern 'def validate_number_of_cores($_: Optional[int], $_: Optional[int])'

Length of output: 1023

Script:

#!/bin/bash
# Let's search for validate_number_of_cores implementation with a broader pattern
rg "def validate_number_of_cores" -A 10

Length of output: 750

cache_directory: Optional[str] = None,
resource_dict: Optional[dict] = None,
flux_executor=None,
Expand Down
15 changes: 10 additions & 5 deletions executorlib/standalone/inputcheck.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,12 +131,14 @@ def check_init_function(block_allocation: bool, init_function: Callable) -> None
raise ValueError("")


def check_max_workers_and_cores(max_workers: int, max_cores: int) -> None:
if max_workers != 1:
def check_max_workers_and_cores(
max_workers: Optional[int], max_cores: Optional[int]
) -> None:
if max_workers is not None:
raise ValueError(
"The number of workers cannot be controlled with the pysqa based backend."
)
if max_cores != 1:
if max_cores is not None:
Comment on lines +135 to +142
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Review needs to be updated - test cases don't align with new optional parameters

The verification revealed an inconsistency between the changes and the test cases:

  • The function check_max_workers_and_cores was modified to make parameters optional (Optional[int])
  • However, the test cases in test_check_max_workers_and_cores still pass non-None values and expect ValueError
  • The actual usage in create_file_executor passes non-None values by default (max_workers=1, max_cores=1)

This indicates that either:

  1. The function shouldn't have been made optional, or
  2. The test cases and usage need to be updated to reflect the new optional behavior
🔗 Analysis chain

LGTM! Type hints and validation logic updated correctly.

The changes properly implement optional parameters while maintaining the original validation purpose. The error messages remain clear and appropriate.

Let's verify the usage of this function across the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for all calls to check_max_workers_and_cores to ensure they handle None correctly
ast-grep --pattern 'check_max_workers_and_cores($$$)'

Length of output: 479

Script:

#!/bin/bash
# Let's check the test file to understand the test cases
cat tests/test_shared_input_check.py

# Also check the executor implementation to see how it handles these parameters
cat executorlib/cache/executor.py

Length of output: 8418

raise ValueError(
"The number of cores cannot be controlled with the pysqa based backend."
)
Expand Down Expand Up @@ -170,6 +172,9 @@ def validate_number_of_cores(max_cores: int, max_workers: int) -> int:
"""
Validate the number of cores and return the appropriate value.
"""
if max_workers != 1 and max_cores == 1:
if max_workers is None and max_cores is None:
return 1
elif max_workers is not None and max_cores is None:
return max_workers
return max_cores
else:
return max_cores
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical: Function needs type safety improvements and better documentation.

Several issues need to be addressed:

  1. The function signature doesn't match the implementation (should use Optional[int])
  2. Missing null checks before using the parameters
  3. Docstring doesn't describe the new behavior with None values

Here's the suggested fix:

- def validate_number_of_cores(max_cores: int, max_workers: int) -> int:
+ def validate_number_of_cores(max_cores: Optional[int], max_workers: Optional[int]) -> int:
     """
     Validate the number of cores and return the appropriate value.
+
+    Args:
+        max_cores: Optional maximum number of cores
+        max_workers: Optional maximum number of workers
+
+    Returns:
+        int: 1 if both parameters are None,
+             max_workers if max_cores is None,
+             max_cores otherwise
     """
     if max_workers is None and max_cores is None:
         return 1
     elif max_workers is not None and max_cores is None:
         return max_workers
-    else:
+    elif max_cores is not None:
         return max_cores
+    else:
+        raise ValueError("Invalid combination of max_cores and max_workers")
📝 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.

Suggested change
if max_workers is None and max_cores is None:
return 1
elif max_workers is not None and max_cores is None:
return max_workers
return max_cores
else:
return max_cores
def validate_number_of_cores(max_cores: Optional[int], max_workers: Optional[int]) -> int:
"""
Validate the number of cores and return the appropriate value.
Args:
max_cores: Optional maximum number of cores
max_workers: Optional maximum number of workers
Returns:
int: 1 if both parameters are None,
max_workers if max_cores is None,
max_cores otherwise
"""
if max_workers is None and max_cores is None:
return 1
elif max_workers is not None and max_cores is None:
return max_workers
elif max_cores is not None:
return max_cores
else:
raise ValueError("Invalid combination of max_cores and max_workers")

Loading