Skip to content

Set max_cores and max_workers to None #478

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

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 all 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

@coderabbitai coderabbitai bot Nov 6, 2024

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
20 changes: 14 additions & 6 deletions executorlib/standalone/inputcheck.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import inspect
import multiprocessing
from concurrent.futures import Executor
from typing import Callable, List, Optional

Expand Down Expand Up @@ -131,12 +132,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

@coderabbitai coderabbitai bot Nov 6, 2024

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 @@ -166,10 +169,15 @@ def check_pysqa_config_directory(pysqa_config_directory: Optional[str]) -> None:
)


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.
"""
if max_workers != 1 and max_cores == 1:
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
return max_cores
else:
return max_cores
12 changes: 6 additions & 6 deletions tests/test_integration_pyiron_workflow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ def slowly_returns_dynamic(dynamic_arg):
return dynamic_arg

dynamic_dynamic = slowly_returns_dynamic()
executor = Executor(block_allocation=True)
executor = Executor(block_allocation=True, max_workers=1)
cloudpickle_register(ind=1)
dynamic_object = does_nothing()
fs = executor.submit(dynamic_dynamic.run, dynamic_object)
Expand Down Expand Up @@ -104,7 +104,7 @@ def slowly_returns_42():
self.assertIsNone(
dynamic_42.result, msg="Just a sanity check that the test is set up right"
)
executor = Executor(block_allocation=True)
executor = Executor(block_allocation=True, max_workers=1)
cloudpickle_register(ind=1)
fs = executor.submit(dynamic_42.run)
fs.add_done_callback(dynamic_42.process_result)
Expand Down Expand Up @@ -135,7 +135,7 @@ def returns_42():
dynamic_42.running,
msg="Sanity check that the test starts in the expected condition",
)
executor = Executor(block_allocation=True)
executor = Executor(block_allocation=True, max_workers=1)
cloudpickle_register(ind=1)
fs = executor.submit(dynamic_42.run)
fs.add_done_callback(dynamic_42.process_result)
Expand All @@ -159,7 +159,7 @@ def raise_error():
raise RuntimeError

re = raise_error()
executor = Executor(block_allocation=True)
executor = Executor(block_allocation=True, max_workers=1)
cloudpickle_register(ind=1)
fs = executor.submit(re.run)
with self.assertRaises(
Expand Down Expand Up @@ -189,7 +189,7 @@ def slowly_returns_dynamic():
return inside_variable

dynamic_dynamic = slowly_returns_dynamic()
executor = Executor(block_allocation=True)
executor = Executor(block_allocation=True, max_workers=1)
cloudpickle_register(ind=1)
fs = executor.submit(dynamic_dynamic.run)
self.assertIsInstance(
Expand Down Expand Up @@ -218,7 +218,7 @@ def slow():
return fortytwo

f = slow()
executor = Executor(block_allocation=True)
executor = Executor(block_allocation=True, max_workers=1)
cloudpickle_register(ind=1)
fs = executor.submit(f.run)
self.assertEqual(
Expand Down
2 changes: 2 additions & 0 deletions tests/test_local_executor_future.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ def submit():
# Executor only exists in this scope and can get garbage collected after
# this function is exits
future = InteractiveExecutor(
max_workers=1,
executor_kwargs={},
spawner=MpiExecSpawner,
).submit(slow_callable)
Expand Down Expand Up @@ -108,6 +109,7 @@ def run(self):
self.running = True

future = InteractiveExecutor(
max_workers=1,
executor_kwargs={},
spawner=MpiExecSpawner,
).submit(self.return_42)
Expand Down
16 changes: 14 additions & 2 deletions tests/test_shared_input_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
check_max_workers_and_cores,
check_hostname_localhost,
check_pysqa_config_directory,
validate_number_of_cores,
)


Expand Down Expand Up @@ -80,9 +81,9 @@ def test_check_flux_executor_pmi_mode(self):

def test_check_max_workers_and_cores(self):
with self.assertRaises(ValueError):
check_max_workers_and_cores(max_workers=2, max_cores=1)
check_max_workers_and_cores(max_workers=2, max_cores=None)
with self.assertRaises(ValueError):
check_max_workers_and_cores(max_workers=1, max_cores=2)
check_max_workers_and_cores(max_workers=None, max_cores=2)
with self.assertRaises(ValueError):
check_max_workers_and_cores(max_workers=2, max_cores=2)

Expand All @@ -95,3 +96,14 @@ def test_check_hostname_localhost(self):
def test_check_pysqa_config_directory(self):
with self.assertRaises(ValueError):
check_pysqa_config_directory(pysqa_config_directory="path/to/config")

def test_validate_number_of_cores(self):
self.assertIsInstance(
validate_number_of_cores(max_cores=None, max_workers=None), int
)
self.assertIsInstance(
validate_number_of_cores(max_cores=1, max_workers=None), int
)
self.assertIsInstance(
validate_number_of_cores(max_cores=None, max_workers=1), int
)
Comment on lines +100 to +109
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 6, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance test coverage and documentation for validate_number_of_cores

While the test verifies return types, consider these improvements:

  1. Add a docstring explaining the test's purpose
  2. Add assertions for expected values, not just types
  3. Include error cases
  4. Test edge cases (e.g., negative values)

Example enhancement:

def test_validate_number_of_cores(self):
    """
    Test validate_number_of_cores function handles various input combinations
    and returns expected core counts.
    """
    # Test return types and values
    self.assertEqual(
        validate_number_of_cores(max_cores=None, max_workers=None),
        os.cpu_count(),  # Expected default
    )
    self.assertEqual(
        validate_number_of_cores(max_cores=2, max_workers=None),
        2,
    )
    self.assertEqual(
        validate_number_of_cores(max_cores=None, max_workers=2),
        os.cpu_count() // 2,  # Expected cores divided by workers
    )
    
    # Test error cases
    with self.assertRaises(ValueError):
        validate_number_of_cores(max_cores=-1, max_workers=None)
    with self.assertRaises(ValueError):
        validate_number_of_cores(max_cores=None, max_workers=-1)

1 change: 1 addition & 0 deletions tests/test_shell_interactive.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ def test_execute_single_task(self):
def test_shell_interactive_executor(self):
cloudpickle_register(ind=1)
with Executor(
max_workers=1,
init_function=init_process,
block_allocation=True,
) as exe:
Expand Down
Loading