Skip to content

Fix working directory #481

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 8, 2024
Merged

Fix working directory #481

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
9c0fc89
Fix working directory
jan-janssen Nov 8, 2024
9afcb56
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 8, 2024
267a8a6
shorter fix
jan-janssen Nov 8, 2024
9da1fed
Merge remote-tracking branch 'origin/working_directory' into working_…
jan-janssen Nov 8, 2024
317397f
even shorted
jan-janssen Nov 8, 2024
efe3302
Merge remote-tracking branch 'refs/remotes/origin/main' into working_…
jan-janssen Nov 8, 2024
2c34f22
another fix
jan-janssen Nov 8, 2024
26305a8
windows fix
jan-janssen Nov 8, 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
1 change: 1 addition & 0 deletions executorlib/cache/shared.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ def execute_tasks_h5(
resource_dict=task_resource_dict,
config_directory=pysqa_config_directory,
backend=backend,
cache_directory=cache_directory,
)
file_name_dict[task_key] = os.path.join(
cache_directory, task_key + ".h5out"
Expand Down
13 changes: 10 additions & 3 deletions executorlib/standalone/cache/queue.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ def execute_with_pysqa(
task_dependent_lst: List[int] = [],
config_directory: Optional[str] = None,
backend: Optional[str] = None,
cache_directory: Optional[str] = None,
) -> int:
"""
Execute a command by submitting it to the queuing system
Expand All @@ -24,12 +25,17 @@ def execute_with_pysqa(
}
config_directory (str, optional): path to the config directory.
backend (str, optional): name of the backend used to spawn tasks.
cache_directory (str): The directory to store the HDF5 files.

Returns:
int: queuing system ID
"""
if resource_dict is None:
resource_dict = {"cwd": "."}
resource_dict = {}
if "cwd" in resource_dict and resource_dict["cwd"] is not None:
cwd = resource_dict["cwd"]
else:
cwd = cache_directory
qa = QueueAdapter(
directory=config_directory,
queue_type=backend,
Expand All @@ -38,9 +44,10 @@ def execute_with_pysqa(
submit_kwargs = {
"command": " ".join(command),
"dependency_list": [str(qid) for qid in task_dependent_lst],
"working_directory": os.path.abspath(resource_dict["cwd"]),
"working_directory": os.path.abspath(cwd),
}
del resource_dict["cwd"]
if "cwd" in resource_dict:
del resource_dict["cwd"]
unsupported_keys = [
"threads_per_core",
"gpus_per_core",
Expand Down
12 changes: 8 additions & 4 deletions executorlib/standalone/cache/spawner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ def execute_in_subprocess(
resource_dict: Optional[dict] = None,
config_directory: Optional[str] = None,
backend: Optional[str] = None,
cache_directory: Optional[str] = None,
) -> subprocess.Popen:
"""
Execute a command in a subprocess.
Expand All @@ -22,6 +23,7 @@ def execute_in_subprocess(
}
config_directory (str, optional): path to the config directory.
backend (str, optional): name of the backend used to spawn tasks.
cache_directory (str): The directory to store the HDF5 files.

Returns:
subprocess.Popen: The subprocess object.
Expand All @@ -38,10 +40,12 @@ def execute_in_subprocess(
if backend is not None:
raise ValueError("backend parameter is not supported for subprocess spawner.")
if resource_dict is None:
resource_dict = {"cwd": None}
elif len(resource_dict) == 0:
resource_dict = {"cwd": None}
return subprocess.Popen(command, universal_newlines=True, cwd=resource_dict["cwd"])
resource_dict = {}
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 8, 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 search results, I can generate the final response as we have found clear evidence of code that relies on the "cwd" key in resource_dict:

The change to empty resource_dict initialization will cause runtime errors

The empty dictionary initialization will break existing code in multiple places:

  • executorlib/standalone/cache/spawner.py directly accesses resource_dict["cwd"] without checking existence
  • executorlib/standalone/cache/queue.py has logic that depends on "cwd" key being present, including:
    • Checking if "cwd" exists and is not None
    • Accessing the "cwd" value
    • Explicitly deleting the "cwd" key

The original initialization with {"cwd": None} was intentional and should be preserved to maintain compatibility with the existing code.

🔗 Analysis chain

Verify the impact of resource_dict initialization change.

The change from initializing with {"cwd": None} to an empty dictionary might affect code that expects the "cwd" key to always exist.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for code that might rely on "cwd" key existence
rg -A 3 'resource_dict.*cwd' --type py

Length of output: 2523

if "cwd" in resource_dict:
cwd = resource_dict["cwd"]
else:
cwd = cache_directory
return subprocess.Popen(command, universal_newlines=True, cwd=cwd)


def terminate_subprocess(task):
Expand Down
Loading