Shared run function for tracking algorithms by surbhigoel77 · Pull Request #174 · Cambridge-ICCS/TCTrack

surbhigoel77 · 2026-04-30T14:52:03Z

The PR aims to build a shared function to run the tracking algorithms. At present, each of the algorithm bindings (tempestextremes.py, tracker.py, tstorms.py) have their own run functions. These functions are part of the python wrappers that take commands (built using make functions) from c++ based algorithms and pass them to OS using subprocess.run to run them. Thus is how we access the external processes.

The commands contain path to the binaries of the algorithm related functions. which can be run subprocess.run

Update:

The code allows 3 verbosity levels (print no o/p, print few lines print, print output real-time)
The code has checks for valid input command set like compatible verbosity value and input_str value.

surbhigoel77 · 2026-05-27T08:29:06Z

@sjavis I will add a unit test for the run function to the same PR

sjavis

This looks good. It is clear there were a few complications in putting it into a single function that I hadn't considered.

I have given some suggestions below. Mostly these are about condensing and simplifying the code.

sjavis · 2026-05-27T16:44:11Z

+                        stdout_lines.append(line)  # also collect it
+
+                    stdout = "".join(stdout_lines)  # ← combine lines into string
+                    stderr, _ = process.communicate()


The order of the return values from process.communicate() is flipped here. stderr should be the second output.

sjavis · 2026-05-27T16:57:11Z

+                    if not command_list:
+                        msg = "command_list cannot be empty"
+                        raise ValueError(msg)


This check should probably be with the initial checks at the start of the function. I don't think it should only be applied for verbosity==0.

sjavis · 2026-05-27T17:04:24Z

+                print(f"{command_name} completed successfully.")
+                print(
+                    f"First 12 lines of output:\n"
+                    f"{''.join(stdout.splitlines(True)[:12])}"
+                    f"\n...\n\n"
+                    f"Last 12 lines of output:\n"
+                    f"{''.join(stdout.splitlines(True)[-12:])}"
+                )


I think the printing of the output is the only difference between verbosity 0 and 1. So I think the better layout for this function to minimise repetition would be:

if verbosity == 2: # The full verbosity code since it is different else: # Run the subprocess and get the outputs if verbosity == 1: # Print the output

sjavis · 2026-05-27T17:08:05Z

-        self._set_metadata()
+            # ── Level 1: Summary — first and last 12 lines ──
+            elif verbosity == 1:
+                print(f"Executing {command_name}...")


This should also be for verbosity == 2. Perhaps move it up to after the initial checks as long as verbosity isn't 0.

sjavis · 2026-05-27T17:13:06Z


-        # Store the global metadata, including the parameters in a json format
-        self._global_metadata = {
-            "tctrack_version": importlib.metadata.version("tctrack"),
-            "tctrack_tracker": type(self).__name__,
-            "tctrack_parameters": json.dumps(parameters),
-        }
+        try:
+            # ── Level 0: Silent — no output ──
+            if verbosity == 0:
+                stdin_file = open(input_file, "r") if input_file is not None else None # noqa: SIM115
+                try:


Since stdin_file is defined in all cases (as stdin for verbosity=2) I think it can be moved above the outer try statement. Then only one finally statement is needed and it removes the additional try blocks

sjavis · 2026-05-28T09:09:17Z

+    class TestRunTrackerSubprocess:
+        """Test suite for run_tracker_subprocess."""
+
+        @pytest.fixture
+        def tracker(self):
+            """Create a tracker instance for testing."""
+            detect_params = TEDetectParameters(in_data=["dummy.nc"])
+            return TETracker(detect_params)


I suggest removing this class and just use use self.ExampleTracker([]) instead of using the tracker fixture.

sjavis · 2026-05-28T09:12:24Z

+            with tempfile.NamedTemporaryFile(
+                mode="w", suffix=".txt", delete=False
+            ) as f:
+                f.write("test\n")
+                temp_file = f.name
+
+            try:
+                with pytest.raises(ValueError):
+                    tracker.run_tracker_subprocess(
+                        command_name="TestCommand",
+                        command_list=["cat"],
+                        input_file=temp_file,
+                        verbosity=5,
+                    )
+            finally:
+                Path(temp_file).unlink()


This test can be simplified because it doesn't need the temporary file.

Suggested change

with tempfile.NamedTemporaryFile(

mode="w", suffix=".txt", delete=False

) as f:

f.write("test\n")

temp_file = f.name

try:

with pytest.raises(ValueError):

tracker.run_tracker_subprocess(

command_name="TestCommand",

command_list=["cat"],

input_file=temp_file,

verbosity=5,

)

finally:

Path(temp_file).unlink()

with pytest.raises(ValueError):

tracker.run_tracker_subprocess(

command_name="TestCommand",

command_list=["cat"],

verbosity=5,

)

sjavis · 2026-05-28T09:18:47Z

+            detect_params = TEDetectParameters(in_data=["dummy.nc"])
+            return TETracker(detect_params)
+
+        def test_run_tracker_subprocess_returns_dict(self, tracker):


It would be good to parametrise this to check the output is correct for different verbosity levels. Make sure to also update the value being passed to run_tracker_subprocess.

Suggested change

def test_run_tracker_subprocess_returns_dict(self, tracker):

@pytest.mark.parametrize("verbosity", [0, 1, 2])

def test_run_tracker_subprocess_returns_dict(self, verbosity):

sjavis · 2026-05-28T09:29:43Z

+
+            finally:
+                Path(temp_file).unlink()
+


I suggest adding a test that the stderr is captured successfully. Eg:

Suggested change

@pytest.mark.parametrize("verbosity", [0, 1, 2])

def test_run_tracker_subprocess_returns_stderr(self, verbosity):

"""Test that function returns correct stderr."""

result = self.ExampleTracker([]).run_tracker_subprocess(

command_name="TestCommand",

command_list=["sh", "-c", 'echo "test error" >&2'],

verbosity=verbosity,

)

assert isinstance(result, dict), "Should return a dict"

assert "stderr" in result, "Dict should have stderr key"

assert "test error" in result["stderr"], "stderr should contain input"

sjavis · 2026-05-28T09:37:34Z

@@ -0,0 +1,93 @@
+"""Test script for verbosity=0 (silent mode)"""


As we discussed, this can be moved into the tests folder. Perhaps merge #199 into this branch and then move it there.

Yes, will add test_run.py to #199 and then merge, just to keep it clean. It was added to this commit by mistake.

Added initial draft shared run function

0964f35

surbhigoel77 changed the title ~~Shared run fucntion for tracking algorithms~~ Shared run function for tracking algorithms Apr 30, 2026

surbhigoel77 marked this pull request as draft April 30, 2026 14:52

surbhigoel77 self-assigned this Apr 30, 2026

surbhigoel77 added 7 commits May 7, 2026 12:33

Validate input type for trackers

89f8686

Validate verbosity value

3295233

Add verbosity 0 operation

8397bb9

Add test script to test new shared run function

573c4c5

Update verbosity 0 functionality and add try-exception

11701e6

Update verbosity 1 functionality

6344a32

Update verbosity 2 functionality

8bf9fb5

surbhigoel77 force-pushed the run_shared branch from 8f6a072 to cd5dfca Compare May 26, 2026 09:40

Correct formatting of run_subprocess function

b844b29

surbhigoel77 force-pushed the run_shared branch from cd5dfca to b844b29 Compare May 27, 2026 08:01

surbhigoel77 requested a review from sjavis May 27, 2026 08:09

surbhigoel77 added 2 commits May 27, 2026 16:01

Add unit test to check the fucntionaing of run subprocess function

4533368

Add unit test to check for invalid verbosity values

c2a5d44

sjavis mentioned this pull request May 28, 2026

Add integration test for tracker #199

Draft

sjavis requested changes May 28, 2026

View reviewed changes

sjavis linked an issue May 29, 2026 that may be closed by this pull request

Shared run function #109

Open

	def test_run_tracker_subprocess_returns_dict(self, tracker):
	@pytest.mark.parametrize("verbosity", [0, 1, 2])
	def test_run_tracker_subprocess_returns_dict(self, verbosity):

+    @pytest.mark.parametrize("verbosity", [0, 1, 2])
+    def test_run_tracker_subprocess_returns_stderr(self, verbosity):
+        """Test that function returns correct stderr."""
+        result = self.ExampleTracker([]).run_tracker_subprocess(
+            command_name="TestCommand",
+            command_list=["sh", "-c", 'echo "test error" >&2'],
+            verbosity=verbosity,
+        )
+        assert isinstance(result, dict), "Should return a dict"
+        assert "stderr" in result, "Dict should have stderr key"
+        assert "test error" in result["stderr"], "stderr should contain input"

		@@ -0,0 +1,93 @@
		"""Test script for verbosity=0 (silent mode)"""

Conversation

surbhigoel77 commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

surbhigoel77 commented May 27, 2026

Uh oh!

sjavis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

surbhigoel77 commented Apr 30, 2026 •

edited

Loading