Skip to content
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

popen options for subprocessing #32

Merged
merged 6 commits into from
Nov 4, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
4 changes: 3 additions & 1 deletion phlop/proc.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@
#
#
#

import subprocess
import sys

from phlop.procs.runtimer import RunTimer


class ProcessNonZeroExitCode(RuntimeError): ...
class ProcessNonZeroExitCode(RuntimeError):
...
PhilipDeegan marked this conversation as resolved.
Show resolved Hide resolved


def run(cmd, shell=True, capture_output=True, check=False, print_cmd=True, **kwargs):
Expand Down
112 changes: 73 additions & 39 deletions phlop/procs/runtimer.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,11 @@
#
#
#
#


import os
import subprocess
import time
from copy import deepcopy

from phlop.os import pushd, write_to_file
from phlop.string import decode_bytes
Expand All @@ -26,63 +24,99 @@ def __init__(
working_dir=None,
log_file_path=None,
logging=2,
popen=True,
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Changing default popen=True may introduce breaking changes

Setting popen=True as the default changes the behavior of RunTimer to use subprocess.Popen instead of subprocess.run. This may affect existing code that relies on the previous default behavior of using subprocess.run.

Consider setting popen=False as the default to maintain backward compatibility, or document this change clearly if the behavior change is intentional.

**kwargs,
):
self.cmd = cmd
start = time.time()

self.run_time = None
self.stdout = ""
self.stderr = ""
self.run_time = None
self.logging = logging
self.log_file_path = log_file_path
self.capture_output = capture_output
benv = os.environ.copy()
benv.update(env)
self.logging = logging

ekwargs = {}
ekwargs = dict(
shell=shell,
env=benv,
close_fds=True,
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
)
if not capture_output and log_file_path:
ekwargs.update(
dict(
stdout=open(f"{log_file_path}.stdout", "w"),
stderr=open(f"{log_file_path}.stderr", "w"),
Comment on lines 49 to 50
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use context managers to safely handle file resources

When opening files with open(), it's best practice to use context managers (with statements) to ensure files are properly closed after use, even if an exception occurs. This prevents potential resource leaks.

Modify the code to manage file handles and ensure they are closed after the subprocess finishes:

-            ekwargs.update(
-                dict(
-                    stdout=open(f"{log_file_path}.stdout", "w"),
-                    stderr=open(f"{log_file_path}.stderr", "w"),
-                ),
-            )
+            stdout_file = open(f"{log_file_path}.stdout", "w")
+            stderr_file = open(f"{log_file_path}.stderr", "w")
+            ekwargs.update(
+                dict(
+                    stdout=stdout_file,
+                    stderr=stderr_file,
+                ),
+            )

Then, after the subprocess completes, ensure the files are closed in _run and _popen methods:

# In _run method, after subprocess.run():
+            if not capture_output and log_file_path:
+                stdout_file.close()
+                stderr_file.close()

# In _popen method, after subprocess.Popen():
+            if not capture_output and log_file_path:
+                stdout_file.close()
+                stderr_file.close()

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 Ruff

49-49: Use a context manager for opening files

(SIM115)


50-50: Use a context manager for opening files

(SIM115)

),
)
else:
ekwargs.update(
dict(stdout=subprocess.PIPE, stderr=subprocess.PIPE),
)

def run():
logging = deepcopy(self.logging)
try:
self.run = subprocess.run(
self.cmd,
shell=shell,
check=check,
env=benv,
capture_output=capture_output,
close_fds=True,
**kwargs,
**ekwargs,
def go():
if popen:
self._popen(*kwargs, **ekwargs)
else:
self._run(
check=check, capture_output=capture_output, *kwargs, **ekwargs
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid star-arg unpacking after keyword arguments

Using *kwargs after keyword arguments is discouraged and can cause unexpected behavior. Adjusting the argument order or unpacking method can prevent this issue.

By replacing *kwargs with **kwargs as suggested, you resolve this concern.

🧰 Tools
🪛 Ruff

63-63: Star-arg unpacking after a keyword argument is strongly discouraged

(B026)

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Incorrect unpacking of kwargs; use **kwargs instead of *kwargs

Unpacking kwargs using *kwargs passes the dictionary keys as positional arguments, which is not intended and can lead to errors. Since kwargs is a dictionary of keyword arguments, it should be unpacked using **kwargs.

Replace *kwargs with **kwargs to correctly pass the additional keyword arguments:

-                self._popen(*kwargs, **ekwargs)
+                self._popen(**kwargs, **ekwargs)

Similarly, adjust the call to _run:

-                self._run(
-                    check=check, capture_output=capture_output, *kwargs, **ekwargs
-                )
+                self._run(
+                    check=check, capture_output=capture_output, **kwargs, **ekwargs
+                )

This change also resolves the static analysis warning about star-arg unpacking after a keyword argument.

📝 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
self._popen(*kwargs, **ekwargs)
else:
self._run(
check=check, capture_output=capture_output, *kwargs, **ekwargs
self._popen(**kwargs, **ekwargs)
else:
self._run(
check=check, capture_output=capture_output, **kwargs, **ekwargs
🧰 Tools
🪛 Ruff

63-63: Star-arg unpacking after a keyword argument is strongly discouraged

(B026)

)
self.run_time = time.time() - start
self.exitcode = self.run.returncode
if logging == 2 and capture_output:
self.stdout = decode_bytes(self.run.stdout)
self.stderr = decode_bytes(self.run.stderr)
except (
subprocess.CalledProcessError
) as e: # only triggers on failure if check=True
self.exitcode = e.returncode
self.run_time = time.time() - start
if logging >= 1 and capture_output:
self.stdout = decode_bytes(e.stdout)
self.stderr = decode_bytes(e.stderr)
logging = 2 # force logging as exception occurred
if logging == 2 and capture_output and log_file_path:
write_to_file(f"{log_file_path}.stdout", self.stdout)
write_to_file(f"{log_file_path}.stderr", self.stderr)

if working_dir:
with pushd(working_dir):
run()
go()
else:
run()
go()

def _locals(self):
return self.capture_output, self.log_file_path, self.logging

def _run(self, **kwargs):
capture_output, log_file_path, logging = self._locals()
try:
start = time.time()
self.run = subprocess.run(self.cmd, **kwargs)
self.run_time = time.time() - start
self.exitcode = self.run.returncode
if logging == 2 and capture_output:
self.stdout = decode_bytes(self.run.stdout)
self.stderr = decode_bytes(self.run.stderr)
except subprocess.CalledProcessError as e:
# only triggers on failure if check=True
self.run_time = time.time() - start
self.exitcode = e.returncode
if logging >= 1 and capture_output:
self.stdout = decode_bytes(e.stdout)
self.stderr = decode_bytes(e.stderr)
logging = 2 # force logging as exception occurred
if logging == 2 and capture_output and log_file_path:
write_to_file(f"{log_file_path}.stdout", self.stdout)
write_to_file(f"{log_file_path}.stderr", self.stderr)

Comment on lines +82 to +96
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor common output handling into a helper method

The output handling logic in both _run and _popen methods is similar. To reduce code duplication and enhance maintainability, consider refactoring this common functionality into a separate helper method.

For example, create a method _handle_output:

def _handle_output(self, stdout, stderr, logging, capture_output, log_file_path):
    if logging == 2 and capture_output:
        self.stdout = decode_bytes(stdout)
        self.stderr = decode_bytes(stderr)
    if logging == 2 and capture_output and log_file_path:
        write_to_file(f"{log_file_path}.stdout", self.stdout)
        write_to_file(f"{log_file_path}.stderr", self.stderr)

Then modify _run and _popen to use this helper:

In _run:

self._handle_output(self.run.stdout, self.run.stderr, logging, capture_output, log_file_path)

In _popen:

self._handle_output(self.stdout, self.stderr, logging, capture_output, log_file_path)

Also applies to: 112-119

def _popen(self, **kwargs):
capture_output, log_file_path, logging = self._locals()
start = time.time()
p = subprocess.Popen(self.cmd, **kwargs)
self.stdout, self.stderr = p.communicate()
self.run_time = time.time() - start
self.exitcode = p.returncode
if not capture_output and log_file_path:
kwargs["stdout"].close()
kwargs["stderr"].close()
elif capture_output:
p.stdout.close()
p.stderr.close()
p = None

if self.exitcode > 0 and capture_output:
logging = 2 # force logging as exception occurred
if logging == 2 and capture_output:
self.stdout = decode_bytes(self.stdout)
self.stderr = decode_bytes(self.stderr)
if logging == 2 and capture_output and log_file_path:
write_to_file(f"{log_file_path}.stdout", self.stdout)
write_to_file(f"{log_file_path}.stderr", self.stderr)

def out(self, ignore_exit_code=False):
if not ignore_exit_code and self.exitcode > 0:
Expand Down
3 changes: 1 addition & 2 deletions phlop/reflection.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
#
#
#
#

import importlib
import inspect
Expand All @@ -17,7 +16,7 @@
).lower() in ("true", "1", "t")


def classes_in_file(file_path, subclasses_only=None, fail_on_import_error=False):
def classes_in_file(file_path, subclasses_only=None, fail_on_import_error=True):
file = Path(file_path)
module = str(file).replace(os.path.sep, ".")[:-3]
assert module
Expand Down
20 changes: 13 additions & 7 deletions phlop/testing/parallel_processor.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,11 @@

from phlop.proc import run

timeout = 60 # seconds - give chance to interrupt
PhilipDeegan marked this conversation as resolved.
Show resolved Hide resolved

class TestCaseFailure(Exception): ...

class TestCaseFailure(Exception):
...


class LoggingMode(Enum):
Expand All @@ -32,7 +35,7 @@ def __call__(self, **kwargs):
self.run = run(
self.test_case.cmd.split(),
shell=False,
capture_output=True,
capture_output=False,
check=True,
print_cmd=False,
env=self.test_case.env,
Expand Down Expand Up @@ -93,10 +96,7 @@ def launch_tests():
)
cc.cores_avail -= batch.cores
cc.procs[batch_index] += [
Process(
target=runner,
args=(test, (pqueue)),
)
Process(target=runner, args=(test, (pqueue)))
PhilipDeegan marked this conversation as resolved.
Show resolved Hide resolved
]
cc.procs[batch_index][-1].daemon = True
cc.procs[batch_index][-1].start()
Expand All @@ -110,7 +110,13 @@ def finished():
def waiter(queue):
fail = 0
while True:
proc = queue.get()
proc = None
try:
proc = queue.get(timeout=timeout)
except Exception:
print("Queue Exception! - no jobs finished - polling")
continue

PhilipDeegan marked this conversation as resolved.
Show resolved Hide resolved
time.sleep(0.01) # don't throttle!
if isinstance(proc, CallableTest):
status = "finished" if proc.run.exitcode == 0 else "FAILED"
Expand Down
7 changes: 4 additions & 3 deletions phlop/testing/test_cases.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
from phlop.sys import extend_sys_path

_LOG_DIR = Path(os.environ.get("PHLOP_LOG_DIR", os.getcwd()))

CMD_PREFIX = ""
CMD_POSTFIX = ""

Expand Down Expand Up @@ -143,8 +142,10 @@ def determine_cores_for_test_case(test_case):


def binless(test_case):
if test_case.cmd.startswith("/usr/bin/"):
test_case.cmd = test_case.cmd[9:]
if test_case.cmd.startswith("/usr/"):
bits = test_case.cmd.split(" ")
test_case.cmd = " ".join([bits[0].split("/")[-1]] + bits[1:])
# print("test_case.cmd ", test_case.cmd)
return test_case


Expand Down
5 changes: 3 additions & 2 deletions phlop/timing/scope_timer.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@
# parsing PHARE scope funtion timers
#

import numpy as np
from pathlib import Path
from dataclasses import dataclass, field
from pathlib import Path

import numpy as np


@dataclass
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[project]
name = "phlop"
version = "0.0.27"
version = "0.0.28"

dependencies = [

Expand Down
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

setup(
name="phlop",
version="0.0.27",
version="0.0.28",
cmdclass={},
classifiers=[],
include_package_data=True,
Expand Down
2 changes: 1 addition & 1 deletion sh/lint.sh
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ set -e

PY_FILES=$(find . -name "*.py")

python3 -m black phlop tests
# python3 -m black phlop tests
pylint --errors-only phlop tests
isort phlop tests
python3 -m ruff check phlop tests
Expand Down
Loading