Skip to content

Move get_command_path() to separate module #438

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 2 commits into from
Oct 26, 2024
Merged

Move get_command_path() to separate module #438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7ec3a50
Move get_command_path() to separate module
jan-janssen Oct 26, 2024
9a0430e
Merge branch 'main' into command
jan-janssen Oct 26, 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/shared/cache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from concurrent.futures import Future
from typing import Any, Tuple

from executorlib.shared.executor import get_command_path
from executorlib.shared.command import get_command_path
from executorlib.shared.hdf import dump, get_output, load
from executorlib.shared.serialize import serialize_funct_h5

Expand Down
14 changes: 14 additions & 0 deletions executorlib/shared/command.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import os


def get_command_path(executable: str) -> str:
"""
Get path of the backend executable script

Args:
executable (str): Name of the backend executable script, either mpiexec.py or serial.py

Returns:
str: absolute path to the executable script
"""
return os.path.abspath(os.path.join(__file__, "..", "..", "backend", executable))
Comment on lines +4 to +14
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 26, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add input validation and path verification.

While the function works, it could benefit from additional safety checks:

  1. Validate the executable parameter against allowed values
  2. Verify the constructed path exists

Here's a suggested improvement:

 def get_command_path(executable: str) -> str:
     """
     Get path of the backend executable script
 
     Args:
-        executable (str): Name of the backend executable script, either mpiexec.py or serial.py
+        executable (str): Name of the backend executable script
+
+    Raises:
+        ValueError: If executable is not one of: mpiexec.py, serial.py
+        FileNotFoundError: If the executable path does not exist
 
     Returns:
         str: absolute path to the executable script
     """
+    allowed_executables = {'mpiexec.py', 'serial.py'}
+    if executable not in allowed_executables:
+        raise ValueError(f"Executable must be one of: {', '.join(allowed_executables)}")
+
     path = os.path.abspath(os.path.join(__file__, "..", "..", "backend", executable))
+    if not os.path.exists(path):
+        raise FileNotFoundError(f"Executable not found at: {path}")
+
     return path
📝 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
def get_command_path(executable: str) -> str:
"""
Get path of the backend executable script
Args:
executable (str): Name of the backend executable script, either mpiexec.py or serial.py
Returns:
str: absolute path to the executable script
"""
return os.path.abspath(os.path.join(__file__, "..", "..", "backend", executable))
def get_command_path(executable: str) -> str:
"""
Get path of the backend executable script
Args:
executable (str): Name of the backend executable script
Raises:
ValueError: If executable is not one of: mpiexec.py, serial.py
FileNotFoundError: If the executable path does not exist
Returns:
str: absolute path to the executable script
"""
allowed_executables = {'mpiexec.py', 'serial.py'}
if executable not in allowed_executables:
raise ValueError(f"Executable must be one of: {', '.join(allowed_executables)}")
path = os.path.abspath(os.path.join(__file__, "..", "..", "backend", executable))
if not os.path.exists(path):
raise FileNotFoundError(f"Executable not found at: {path}")
return path

Copy link
Contributor

@coderabbitai coderabbitai bot Oct 26, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Path resolution logic needs correction

The current implementation has issues:

  1. The path resolution logic in get_command_path references backend/mpiexec.py or backend/serial.py in its docstring, but the actual files being accessed are:

    • executorlib/backend/interactive_parallel.py
    • executorlib/backend/interactive_serial.py
    • executorlib/backend/cache_parallel.py
    • executorlib/backend/cache_serial.py

  2. The path resolution os.path.join(__file__, "..", "..", "backend", executable) assumes a different directory structure than what actually exists. The backend files are located in executorlib/backend/ relative to the repository root, not in a top-level backend/ directory.

🔗 Analysis chain

Verify the path resolution logic across the codebase.

Let's ensure the path resolution works correctly in different contexts where this function might be used.

Let me gather more information about the backend directory structure and the actual Python files being referenced.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the backend directory structure and usage of get_command_path

# Test 1: Check if backend directory exists and contains expected files
echo "Checking backend directory structure..."
fd -t f -p "backend/(mpiexec|serial)\.py$"

# Test 2: Find all references to get_command_path to understand usage patterns
echo -e "\nChecking get_command_path usage..."
rg "get_command_path\(" -A 2

Length of output: 1247

Script:

#!/bin/bash
# Check the actual backend directory content and verify the Python files
echo "Listing all files in backend directory:"
fd . "backend/"

echo -e "\nChecking content of the Python files to verify they exist and are properly referenced:"
for file in "interactive_parallel.py" "interactive_serial.py" "cache_parallel.py" "cache_serial.py"; do
    echo -e "\nChecking $file:"
    fd -t f "$file"
done

# Check the actual implementation of get_command_path function
echo -e "\nChecking get_command_path implementation:"
rg -A 5 "def get_command_path" executorlib/shared/command.py

Length of output: 1746

15 changes: 1 addition & 14 deletions executorlib/shared/executor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import importlib.util
import inspect
import os
import queue
import sys
from concurrent.futures import (
Expand All @@ -14,6 +13,7 @@

import cloudpickle

from executorlib.shared.command import get_command_path
from executorlib.shared.communication import interface_bootup
from executorlib.shared.inputcheck import (
check_resource_dict,
Expand Down Expand Up @@ -465,19 +465,6 @@ def execute_tasks_with_dependencies(
sleep(refresh_rate)


def get_command_path(executable: str) -> str:
"""
Get path of the backend executable script

Args:
executable (str): Name of the backend executable script, either mpiexec.py or serial.py

Returns:
str: absolute path to the executable script
"""
return os.path.abspath(os.path.join(__file__, "..", "..", "backend", executable))


def _get_backend_path(
cores: int,
) -> list:
Expand Down
Loading