Skip to content

Change Backend Names #500

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
jan-janssen merged 8 commits into from
Nov 15, 2024
Merged

Change Backend Names #500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7dfe655
Change Backend Names
jan-janssen Nov 15, 2024
0e56890
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 15, 2024
9ad5619
Update __init__.py
jan-janssen Nov 15, 2024
1769ab4
fix flux tests
jan-janssen Nov 15, 2024
7c5ccab
fix flux submission test
jan-janssen Nov 15, 2024
661f756
fix input checks
jan-janssen Nov 15, 2024
93bd0be
fix notebooks
jan-janssen Nov 15, 2024
5cce4fc
Add more tests
jan-janssen Nov 15, 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
2 changes: 1 addition & 1 deletion executorlib/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ def __new__(
resource_dict.update(
{k: v for k, v in default_resource_dict.items() if k not in resource_dict}
)
if "pysqa_" in backend and not plot_dependency_graph:
if "_submission" in backend and not plot_dependency_graph:
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 15, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Based on the shell script results, I can now generate the final response:

Backend naming convention changes are partially implemented

The codebase shows an inconsistent state of backend naming conventions:

  • New _submission and _allocation suffixes are used in the core implementation
  • However, there are still multiple references to the old pysqa_ prefix, particularly in:
    • Core functionality: executorlib/cache/queue_spawner.py (_pysqa_execute_command)
    • Configuration handling: executorlib/standalone/inputcheck.py (check_pysqa_config_directory)
    • Multiple parameter references in docstrings and type hints across the codebase
🔗 Analysis chain

Verify backend naming convention changes across the codebase

The change from checking "pysqa_" to "_submission" in the backend string represents a significant change in naming conventions. Let's verify this change is consistently applied across the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining instances of old backend naming and verify new naming convention usage

# Check for any remaining "pysqa_" backend references
echo "Checking for old 'pysqa_' backend references:"
rg "pysqa_" --type py

# Check usage of new "_submission" backend naming
echo -e "\nChecking new '_submission' backend usage:"
rg "_submission" --type py

# Check usage of new "_allocation" backend naming
echo -e "\nChecking new '_allocation' backend usage:"
rg "_allocation" --type py

Length of output: 9620

from executorlib.cache.executor import create_file_executor

return create_file_executor(
Expand Down
4 changes: 2 additions & 2 deletions executorlib/cache/executor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ def __init__(

def create_file_executor(
max_workers: int = 1,
backend: str = "pysqa_flux",
backend: str = "flux_submission",
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 15, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Update test and error messages for backend naming convention change

The verification reveals several issues that need to be addressed:

  • The test file tests/test_cache_executor_pysqa_flux.py still uses the old pysqa_flux backend name
  • Multiple error messages across the codebase reference "pysqa based backend" which is outdated
  • The codebase shows a clear transition to the new *_submission naming convention

Required changes:

  • Rename tests/test_cache_executor_pysqa_flux.py to match the new naming convention and update its content
  • Update error messages in:
    • executorlib/cache/executor.py
    • executorlib/cache/shared.py
    • executorlib/standalone/inputcheck.py
    • executorlib/__init__.py
🔗 Analysis chain

Breaking change: Verify impact of backend name change

The default backend value has changed from "pysqa_flux" to "flux_submission", which could break existing code. Additionally, the error messages still reference "pysqa based backend" which is inconsistent with the new naming convention.

Let's verify the impact and consistency:

Consider:

  1. Adding a deprecation warning for the old backend name
  2. Updating error messages to remove "pysqa based backend" references
  3. Updating documentation to reflect the new naming convention
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for direct usage of the old backend name
rg "pysqa_flux"

# Check for references to pysqa in error messages and comments
rg -l "pysqa.*backend"

# Check for consistency in backend naming across the codebase
rg "_submission[\"']|\.[\"']submission"

Length of output: 517

Script:

#!/bin/bash
# Let's check the content of the test file to understand the test context
cat tests/test_cache_executor_pysqa_flux.py

# Check the actual error messages containing pysqa references
rg -A 2 "pysqa.*backend" executorlib/cache/executor.py
rg -A 2 "pysqa.*backend" executorlib/cache/shared.py
rg -A 2 "pysqa.*backend" executorlib/standalone/inputcheck.py
rg -A 2 "pysqa.*backend" executorlib/__init__.py

# Check if there are any other backend types being used
rg "backend\s*=\s*[\"'][^\"']*[\"']"

Length of output: 6194

max_cores: int = 1,
cache_directory: Optional[str] = None,
resource_dict: Optional[dict] = None,
Expand Down Expand Up @@ -113,6 +113,6 @@ def create_file_executor(
cache_directory=cache_directory,
resource_dict=resource_dict,
pysqa_config_directory=pysqa_config_directory,
backend=backend.split("pysqa_")[-1],
backend=backend.split("_submission")[0],
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 15, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation for backend name format

The current implementation assumes all backend names will contain "_submission" without validation. This could lead to unexpected behavior if an invalid backend name is provided.

Consider adding validation and proper error handling:

-        backend=backend.split("_submission")[0],
+        backend=backend.split("_submission")[0] if "_submission" in backend else 
+            raise ValueError(f"Invalid backend format: {backend}. Expected format: <backend>_submission")

This ensures that:

  1. The backend name follows the expected format
  2. Invalid formats are caught early with a clear error message

Committable suggestion skipped: line range outside the PR's diff.

disable_dependencies=disable_dependencies,
)
14 changes: 9 additions & 5 deletions executorlib/interactive/executor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,13 +198,13 @@ def create_executor(
init_function (None): optional function to preset arguments for functions which are submitted later
"""
check_init_function(block_allocation=block_allocation, init_function=init_function)
if flux_executor is not None and backend != "flux":
backend = "flux"
if flux_executor is not None and backend != "flux_allocation":
backend = "flux_allocation"
check_pmi(backend=backend, pmi=flux_executor_pmi_mode)
cores_per_worker = resource_dict["cores"]
resource_dict["cache_directory"] = cache_directory
resource_dict["hostname_localhost"] = hostname_localhost
if backend == "flux":
if backend == "flux_allocation":
check_oversubscribe(oversubscribe=resource_dict["openmpi_oversubscribe"])
check_command_line_argument_lst(
command_line_argument_lst=resource_dict["slurm_cmd_args"]
Expand Down Expand Up @@ -233,7 +233,7 @@ def create_executor(
executor_kwargs=resource_dict,
spawner=FluxPythonSpawner,
)
elif backend == "slurm":
elif backend == "slurm_allocation":
check_executor(executor=flux_executor)
check_nested_flux_executor(nested_flux_executor=flux_executor_nesting)
if block_allocation:
Expand All @@ -255,7 +255,7 @@ def create_executor(
executor_kwargs=resource_dict,
spawner=SrunSpawner,
)
else: # backend="local"
elif backend == "local":
check_executor(executor=flux_executor)
check_nested_flux_executor(nested_flux_executor=flux_executor_nesting)
check_threads_per_core(threads_per_core=resource_dict["threads_per_core"])
Expand Down Expand Up @@ -285,3 +285,7 @@ def create_executor(
executor_kwargs=resource_dict,
spawner=MpiExecSpawner,
)
else:
raise ValueError(
"The supported backends are slurm_allocation, slurm_submission, flux_allocation, flux_submission and local."
)
Comment on lines +288 to +291
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 15, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Unsupported backends mentioned in error message

The error message lists 'slurm_submission' and 'flux_submission' as supported backends, but there is no handling for these backends in the code above. This could lead to confusion if a user specifies one of these backends.

Either implement handling for 'slurm_submission' and 'flux_submission' backends or update the error message to reflect the actual supported backends. This will ensure users receive accurate information.

Apply this diff to update the error message:

            raise ValueError(
-                "The supported backends are slurm_allocation, slurm_submission, flux_allocation, flux_submission and local."
+                "The supported backends are slurm_allocation, flux_allocation, and local."
            )
📝 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
else:
raise ValueError(
"The supported backends are slurm_allocation, slurm_submission, flux_allocation, flux_submission and local."
)
else:
raise ValueError(
"The supported backends are slurm_allocation, flux_allocation, and local."
)

4 changes: 2 additions & 2 deletions executorlib/standalone/inputcheck.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,9 @@ def check_pmi(backend: str, pmi: Optional[str]) -> None:
"""
Check if pmi is valid for the selected backend and raise a ValueError if it is not.
"""
if backend != "flux" and pmi is not None:
if backend != "flux_allocation" and pmi is not None:
raise ValueError("The pmi parameter is currently only implemented for flux.")
elif backend == "flux" and pmi not in ["pmix", "pmi1", "pmi2", None]:
elif backend == "flux_allocation" and pmi not in ["pmix", "pmi1", "pmi2", None]:
Comment on lines +120 to +122
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 15, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Backend name inconsistency detected - update required

Several files still use the old "flux" backend name instead of "flux_allocation":

  • executorlib/__init__.py: Documentation and code references "flux" in backend options
  • executorlib/interactive/executor.py: Documentation lists "flux" as a backend option
  • tests/test_shared_input_check.py: Uses "flux" in test cases
  • tests/test_cache_executor_serial.py: Uses "flux" in test execution
  • tests/benchmark/llh.py: Uses "flux" for backend configuration

These instances need to be updated to use "flux_allocation" for consistency with the new implementation.

🔗 Analysis chain

Verify consistent usage of new backend name.

Let's ensure the new backend name "flux_allocation" is used consistently across the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining instances of the old "flux" backend name
# and verify consistent usage of "flux_allocation"

echo "Checking for old 'flux' backend references..."
rg --type python '"flux"' -C 2 | grep -v "flux_allocation" | grep -v "flux_submission"

echo "Verifying usage of new 'flux_allocation' backend..."
rg --type python '"flux_allocation"' -C 2

Length of output: 416

Script:

#!/bin/bash
# Let's try without specifying file type since Python extension isn't recognized
echo "Checking for old 'flux' backend references..."
rg '"flux"' -C 2 | grep -v "flux_allocation" | grep -v "flux_submission"

echo -e "\nVerifying usage of new 'flux_allocation' backend..."
rg '"flux_allocation"' -C 2

Length of output: 7580

raise ValueError(
"The pmi parameter supports [pmix, pmi1, pmi2], but not: " + pmi
)
Expand Down
22 changes: 11 additions & 11 deletions notebooks/examples.ipynb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
"source": [
"from executorlib import Executor\n",
"\n",
"with Executor(max_cores=1, backend=\"flux\") as exe:\n",
"with Executor(max_cores=1, backend=\"flux_allocation\") as exe:\n",
" future = exe.submit(sum, [1, 1])\n",
" print(future.result())"
]
Expand Down Expand Up @@ -103,7 +103,7 @@
" return sum(*args)\n",
"\n",
"\n",
"with Executor(max_cores=2, backend=\"flux\") as exe:\n",
"with Executor(max_cores=2, backend=\"flux_allocation\") as exe:\n",
" fs_1 = exe.submit(calc, [2, 1])\n",
" fs_2 = exe.submit(calc, [2, 2])\n",
" fs_3 = exe.submit(calc, [2, 3])\n",
Expand Down Expand Up @@ -159,7 +159,7 @@
" return sum(*args)\n",
"\n",
"\n",
"with Executor(max_cores=2, backend=\"flux\") as exe:\n",
"with Executor(max_cores=2, backend=\"flux_allocation\") as exe:\n",
" print(list(exe.map(calc, [[2, 1], [2, 2], [2, 3], [2, 4]])))"
]
},
Expand Down Expand Up @@ -205,7 +205,7 @@
" with Executor(\n",
" # Resource definition on the executor level\n",
" max_workers=2, # total number of cores available to the Executor\n",
" backend=\"flux\", # optional in case the backend is not recognized\n",
" backend=\"flux_allocation\", # optional in case the backend is not recognized\n",
" # Optional resource definition\n",
" resource_dict={\n",
" \"cores\": 1,\n",
Expand Down Expand Up @@ -277,7 +277,7 @@
" # Resource definition on the executor level\n",
" max_cores=2, # total number of cores available to the Executor\n",
" block_allocation=True, # reuse python processes\n",
" backend=\"flux\",\n",
" backend=\"flux_allocation\",\n",
") as exe:\n",
" future_obj = exe.submit(\n",
" calc_function,\n",
Expand Down Expand Up @@ -332,7 +332,7 @@
"with Executor(\n",
" max_cores=1,\n",
" init_function=init_function,\n",
" backend=\"flux\",\n",
" backend=\"flux_allocation\",\n",
" block_allocation=True,\n",
") as exe:\n",
" fs = exe.submit(calc, 2, j=5)\n",
Expand Down Expand Up @@ -462,7 +462,7 @@
"with Executor(\n",
" max_cores=2,\n",
" resource_dict={\"cores\": 2},\n",
" backend=\"flux\",\n",
" backend=\"flux_allocation\",\n",
" flux_executor_pmi_mode=\"pmix\",\n",
") as exe:\n",
" fs = exe.submit(calc, 3)\n",
Expand Down Expand Up @@ -519,7 +519,7 @@
"with Executor(\n",
" max_workers=2, \n",
" gpus_per_worker=1,\n",
" backend=\"flux\",\n",
" backend=\"flux_allocation\",\n",
") as exe:\n",
" fs_1 = exe.submit(get_available_gpus)\n",
" fs_2 = exe.submit(get_available_gpus)\n",
Expand Down Expand Up @@ -627,7 +627,7 @@
" return parameter_a + parameter_b\n",
"\n",
"\n",
"with Executor(max_cores=2, backend=\"flux\") as exe:\n",
"with Executor(max_cores=2, backend=\"flux_allocation\") as exe:\n",
" future_1 = exe.submit(\n",
" calc_function,\n",
" 1,\n",
Expand Down Expand Up @@ -672,7 +672,7 @@
"```\n",
"from executorlib import Executor\n",
"\n",
"with Executor(max_cores=1, backend=\"slurm\") as exe:\n",
"with Executor(max_cores=1, backend=\"slurm_allocation\") as exe:\n",
" future = exe.submit(sum, [1,1])\n",
" print(future.result())\n",
"```"
Expand All @@ -683,7 +683,7 @@
"id": "ae8dd860-f90f-47b4-b3e5-664f5c949350",
"metadata": {},
"source": [
"The `backend=\"slurm\"` parameter is optional as `executorlib` automatically recognizes if [flux framework](https://flux-framework.org)\n",
"The `backend=\"slurm_allocation\"` parameter is optional as `executorlib` automatically recognizes if [flux framework](https://flux-framework.org)\n",
"or SLURM are available. \n",
"\n",
Comment on lines +686 to 688
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 15, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix documentation inconsistency with backend name

The documentation mentions backend="slurm_allocation" but all previous examples use backend="flux_allocation". This inconsistency could confuse users.

Apply this diff to fix the inconsistency:

-The `backend="slurm_allocation"` parameter is optional as `executorlib` automatically recognizes if [flux framework](https://flux-framework.org)
+The `backend="flux_allocation"` parameter is optional as `executorlib` automatically recognizes if [flux framework](https://flux-framework.org)
📝 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
"The `backend=\"slurm_allocation\"` parameter is optional as `executorlib` automatically recognizes if [flux framework](https://flux-framework.org)\n",
"or SLURM are available. \n",
"\n",
"The `backend=\"flux_allocation\"` parameter is optional as `executorlib` automatically recognizes if [flux framework](https://flux-framework.org)\n",
"or SLURM are available. \n",
"\n",

"In addition, the SLURM backend introduces the `command_line_argument_lst=[]` parameter, which allows the user to provide\n",
Expand Down
2 changes: 1 addition & 1 deletion tests/test_cache_executor_pysqa_flux.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def mpi_funct(i):
class TestCacheExecutorPysqa(unittest.TestCase):
def test_executor(self):
with Executor(
backend="pysqa_flux",
backend="flux_submission",
resource_dict={"cores": 2, "cwd": "cache"},
block_allocation=False,
cache_directory="cache",
Expand Down
4 changes: 4 additions & 0 deletions tests/test_dependencies_executor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ def test_executor_dependency_plot(self):
self.assertEqual(len(nodes), 5)
self.assertEqual(len(edges), 4)

def test_create_executor_error(self):
with self.assertRaises(ValueError):
create_executor(backend="toast", resource_dict={"cores": 1})

def test_dependency_steps(self):
cloudpickle_register(ind=1)
fs1 = Future()
Expand Down
10 changes: 5 additions & 5 deletions tests/test_executor_backend_flux.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def test_flux_executor_serial(self):
with Executor(
max_cores=2,
flux_executor=self.executor,
backend="flux",
backend="flux_allocation",
block_allocation=True,
) as exe:
fs_1 = exe.submit(calc, 1)
Expand All @@ -62,7 +62,7 @@ def test_flux_executor_threads(self):
max_cores=1,
resource_dict={"threads_per_core": 2},
flux_executor=self.executor,
backend="flux",
backend="flux_allocation",
block_allocation=True,
) as exe:
fs_1 = exe.submit(calc, 1)
Expand All @@ -77,7 +77,7 @@ def test_flux_executor_parallel(self):
max_cores=2,
resource_dict={"cores": 2},
flux_executor=self.executor,
backend="flux",
backend="flux_allocation",
block_allocation=True,
flux_executor_pmi_mode=pmi,
) as exe:
Expand All @@ -90,7 +90,7 @@ def test_single_task(self):
max_cores=2,
resource_dict={"cores": 2},
flux_executor=self.executor,
backend="flux",
backend="flux_allocation",
block_allocation=True,
flux_executor_pmi_mode=pmi,
) as p:
Expand All @@ -106,7 +106,7 @@ def test_internal_memory(self):
resource_dict={"cores": 1},
init_function=set_global,
flux_executor=self.executor,
backend="flux",
backend="flux_allocation",
block_allocation=True,
) as p:
f = p.submit(get_global)
Expand Down
2 changes: 1 addition & 1 deletion tests/test_shared_input_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def test_check_pmi(self):
with self.assertRaises(ValueError):
check_pmi(backend="test", pmi="test")
with self.assertRaises(ValueError):
check_pmi(backend="flux", pmi="test")
check_pmi(backend="flux_allocation", pmi="test")
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 15, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Backend name "flux" still exists in some test files and needs to be updated

Found inconsistent backend naming that needs attention:

  • tests/test_cache_executor_serial.py: Uses backend="flux"
  • tests/benchmark/llh.py: Uses backend="flux"

These files should be updated to use backend="flux_allocation" for consistency with the changes in other test files.

🔗 Analysis chain

Verify consistent backend name updates across the codebase.

Let's ensure the backend name change from "flux" to "flux_allocation" is consistently applied.

Let me try another search strategy without relying on file type detection.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining instances of the old "flux" backend name
# and verify the new "flux_allocation" name is used consistently

# Search for any remaining standalone "flux" backend references
echo "Checking for old 'flux' backend references:"
rg --type python 'backend="flux"' || rg --type python "backend='flux'"

# Search for the new "flux_allocation" backend references to confirm consistent usage
echo -e "\nVerifying new 'flux_allocation' backend usage:"
rg --type python 'backend="flux_allocation"' || rg --type python "backend='flux_allocation'"

# Search for any flux-related configuration or backend definitions
echo -e "\nChecking backend definitions and configurations:"
ast-grep --pattern 'backend = "flux"' || ast-grep --pattern "backend = 'flux'"

Length of output: 672

Script:

#!/bin/bash
# Search for any remaining standalone "flux" backend references
echo "Checking for old 'flux' backend references:"
rg 'backend="flux"' || rg "backend='flux'"

# Search for the new "flux_allocation" backend references
echo -e "\nVerifying new 'flux_allocation' backend usage:"
rg 'backend="flux_allocation"' || rg "backend='flux_allocation'"

# Search for flux-related imports or configurations
echo -e "\nChecking flux-related imports and configurations:"
rg -A 2 'from.*flux' || rg -A 2 'import.*flux'

Length of output: 2478


def test_check_nested_flux_executor(self):
with self.assertRaises(ValueError):
Expand Down
Loading