Skip to content

Conversation

@tomc271
Copy link

@tomc271 tomc271 commented Jan 21, 2026

Use subprocess.run() instead of subprocess.Popen(),
setting environment variables set with the env parameter.

This change is required in order for tests to by run in parallel via pytest and xdist.

@tomc271 tomc271 requested a review from ZedThree January 21, 2026 10:28
Copy link
Contributor

@dschwoerer dschwoerer left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +55 to +57
if result.stderr:
output += "\nSTDERR:\n" + result.stderr
return result.returncode, output
Copy link
Member

Choose a reason for hiding this comment

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

Let's just switch to an f-string here:

Suggested change
if result.stderr:
output += "\nSTDERR:\n" + result.stderr
return result.returncode, output
if result.stderr:
output = f"{output}\nSTDERR:\n{result.stderr}"
return result.returncode, output

else:
cmd = f"OMP_NUM_THREADS={mthread} {cmd}"
# Set OMP_NUM_THREADS if mthread is provided (for OpenMP in BOUT++)
env = dict(os.environ)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't os.environ already a dict? Or is this just to make a copy?

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.

5 participants