Skip to content

Add cwd option to add_process #469

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.

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 5 commits into from
Aug 2, 2024
Merged

Add cwd option to add_process #469

merged 5 commits into from
Aug 2, 2024

Conversation

wk9874
Copy link
Collaborator

@wk9874 wk9874 commented Jul 30, 2024

For the FDS connector to be able to support executing a simulation in a different working directory to the place where the connector is called, need to be able to specify a working directory for the process to use (since FDS always puts results into the working directory). Added this as an option to the add_process method, which then just passes the cwd parameter through to the subprocess.

This now assumes that the path to the script or input_file are relative to the cwd parameter, so have updated the file upload in add_process to join together these paths before trying to find the file if cwd is specified. Also had to loosen the typing for script and input_file to accept a string or Path, since the path provided for either of these may not exist if cwd is specified.

Added a test which checks this functionality

@wk9874 wk9874 added this to the API v1.1 milestone Jul 30, 2024
@wk9874 wk9874 requested a review from kzscisoft July 30, 2024 08:27
@@ -203,10 +208,20 @@ def callback_function(status_code: int, std_out: str, std_err: str) -> None:
)

if script:
self._runner.save_file(file_path=script, category="code")
if cwd:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can simplify this to:

self._runner.save_file(
    file_path=os.path.join(cwd or os.getcwd(), script), category="code"
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My issue with this would be what if they've provided the absolute path to the script? Will it not then duplicate the start of the path and fail to find the file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well the issue then is that someone could provide both cwd and the path to their script as an absolute path.
I don't really agree with script being a string this creates ambiguity:

  • cwd should be directory to run process in.
  • script should be a path to the script to execute.

The two are not related as you might want to run a script from another directory in the current directory. I think the solution here is unstable

Copy link
Collaborator

@kzscisoft kzscisoft left a comment

Choose a reason for hiding this comment

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

Looks good, this can go in

@kzscisoft kzscisoft merged commit 71a85b2 into dev Aug 2, 2024
14 checks passed
@kzscisoft kzscisoft deleted the add_process_cwd branch August 2, 2024 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants