From 8e5aa72baea29932f1c162f2523089dae240f406 Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Mon, 22 Apr 2024 13:54:44 -0700 Subject: [PATCH 01/22] refactor(performance-metrics): pull out storage logic into class --- api/tests/opentrons/cli/test_cli.py | 38 ++++++++-- .../src/performance_metrics/data_store.py | 33 +++++++++ .../src/performance_metrics/datashapes.py | 42 +++++++++-- .../robot_context_tracker.py | 41 ++++++----- .../performance_metrics/test_data_store.py | 0 .../test_robot_context_tracker.py | 72 ++++++++++--------- .../performance/dev_types.py | 26 ++++++- 7 files changed, 186 insertions(+), 66 deletions(-) create mode 100644 performance-metrics/src/performance_metrics/data_store.py create mode 100644 performance-metrics/tests/performance_metrics/test_data_store.py diff --git a/api/tests/opentrons/cli/test_cli.py b/api/tests/opentrons/cli/test_cli.py index 007a7dd6a03..83d9468347e 100644 --- a/api/tests/opentrons/cli/test_cli.py +++ b/api/tests/opentrons/cli/test_cli.py @@ -5,12 +5,15 @@ import tempfile import textwrap -from dataclasses import dataclass +from dataclasses import dataclass, replace from typing import Any, Dict, Iterator, List, Optional from pathlib import Path import pytest from click.testing import CliRunner +from opentrons_shared_data.performance.dev_types import ( + RobotContextState, +) from opentrons.util.performance_helpers import _get_robot_context_tracker @@ -24,6 +27,18 @@ from opentrons.cli.analyze import analyze # noqa: E402 +@pytest.fixture +def override_data_store(tmp_path: Path) -> Iterator[None]: + """Override the data store metadata for the RobotContextTracker.""" + old_store = context_tracker._store # type: ignore[attr-defined] + old_metadata = old_store.metadata + new_metadata = replace(old_metadata, storage_dir=tmp_path) + context_tracker._store = old_store.__class__(metadata=new_metadata) # type: ignore[attr-defined] + context_tracker._store.setup() # type: ignore[attr-defined] + yield + context_tracker._store = old_store # type: ignore[attr-defined] + + def _list_fixtures(version: int) -> Iterator[Path]: return Path(__file__).parent.glob( f"../../../../shared-data/protocol/fixtures/{version}/*.json" @@ -255,6 +270,7 @@ def test_python_error_line_numbers( assert error["detail"] == expected_detail +@pytest.mark.usefixtures("override_data_store") def test_track_analysis(tmp_path: Path) -> None: """Test that the RobotContextTracker tracks analysis.""" protocol_source = textwrap.dedent( @@ -265,12 +281,26 @@ def run(protocol): pass """ ) - protocol_source_file = tmp_path / "protocol.py" protocol_source_file.write_text(protocol_source, encoding="utf-8") + store = context_tracker._store # type: ignore[attr-defined] - before_analysis = len(context_tracker._storage) # type: ignore[attr-defined] + num_storage_entities_before_analysis = len(store.data) _get_analysis_result([protocol_source_file]) - assert len(context_tracker._storage) == before_analysis + 1 # type: ignore[attr-defined] + assert len(store.data) == num_storage_entities_before_analysis + 1 + + with open(store.metadata.data_file_location, "r") as f: + stored_data = f.readlines() + assert len(stored_data) == 0 + + context_tracker.store() + + with open(store.metadata.data_file_location, "r") as f: + stored_data = f.readlines() + assert len(stored_data) == 1 + state_id, start_time, duration = stored_data[0].strip().split(",") + assert state_id == str(RobotContextState.ANALYZING_PROTOCOL.state_id) + assert start_time.isdigit() + assert duration.isdigit() diff --git a/performance-metrics/src/performance_metrics/data_store.py b/performance-metrics/src/performance_metrics/data_store.py new file mode 100644 index 00000000000..b64ed4021e3 --- /dev/null +++ b/performance-metrics/src/performance_metrics/data_store.py @@ -0,0 +1,33 @@ +"""Interface for storing performance metrics data to a CSV file.""" + +import csv +import typing +from opentrons_shared_data.performance.dev_types import StorageMetadata +from performance_metrics.datashapes import SupportsCSVStorage + +T = typing.TypeVar("T", bound=SupportsCSVStorage) + + +class MetricsStore(typing.Generic[T]): + """Dataclass to store data for tracking robot context.""" + + def __init__(self, metadata: StorageMetadata) -> None: + """Initialize the storage engine.""" + self.metadata = metadata + self.data: typing.List[T] = [] + + def setup(self) -> None: + """Set up the data store.""" + self.metadata.storage_dir.mkdir(parents=True, exist_ok=True) + self.metadata.data_file_location.touch(exist_ok=True) + self.metadata.headers_file_location.touch(exist_ok=True) + self.metadata.headers_file_location.write_text(",".join(self.metadata.headers)) + + def store(self) -> None: + """Clear the stored data and write it to the storage file.""" + stored_data = self.data.copy() + self.data.clear() + rows_to_write = [context_data.csv_row() for context_data in stored_data] + with open(self.metadata.data_file_location, "a") as storage_file: + writer = csv.writer(storage_file) + writer.writerows(rows_to_write) diff --git a/performance-metrics/src/performance_metrics/datashapes.py b/performance-metrics/src/performance_metrics/datashapes.py index 7743ed1723d..d97d51fcb2a 100644 --- a/performance-metrics/src/performance_metrics/datashapes.py +++ b/performance-metrics/src/performance_metrics/datashapes.py @@ -1,12 +1,32 @@ -"""Defines data classes and enums used in the performance metrics module.""" +"""Defines the shape of stored data.""" import dataclasses -from typing import Tuple +from typing import Sequence, Tuple, Protocol, Union from opentrons_shared_data.performance.dev_types import RobotContextState +StorableData = Union[int, float, str] + + +class SupportsCSVStorage(Protocol): + """A protocol for classes that support CSV storage.""" + + @classmethod + def headers(self) -> Tuple[str, ...]: + """Returns the headers for the CSV data.""" + ... + + def csv_row(self) -> Tuple[StorableData, ...]: + """Returns the object as a CSV row.""" + ... + + @classmethod + def from_csv_row(cls, row: Tuple[StorableData, ...]) -> "SupportsCSVStorage": + """Returns an object from a CSV row.""" + ... + @dataclasses.dataclass(frozen=True) -class RawContextData: +class RawContextData(SupportsCSVStorage): """Represents raw duration data with context state information. Attributes: @@ -16,10 +36,9 @@ class RawContextData: - state (RobotContextStates): The current state of the context. """ - func_start: int - duration_start: int - duration_end: int state: RobotContextState + func_start: int + duration: int @classmethod def headers(self) -> Tuple[str, str, str]: @@ -31,5 +50,14 @@ def csv_row(self) -> Tuple[int, int, int]: return ( self.state.state_id, self.func_start, - self.duration_end - self.duration_start, + self.duration, + ) + + @classmethod + def from_csv_row(cls, row: Sequence[StorableData]) -> SupportsCSVStorage: + """Returns a RawContextData object from a CSV row.""" + return cls( + state=RobotContextState.from_id(int(row[0])), + func_start=int(row[1]), + duration=int(row[2]), ) diff --git a/performance-metrics/src/performance_metrics/robot_context_tracker.py b/performance-metrics/src/performance_metrics/robot_context_tracker.py index 99dc502c9ad..7e310126837 100644 --- a/performance-metrics/src/performance_metrics/robot_context_tracker.py +++ b/performance-metrics/src/performance_metrics/robot_context_tracker.py @@ -1,23 +1,22 @@ """Module for tracking robot context and execution duration for different operations.""" -import csv from pathlib import Path import platform from functools import wraps, partial from time import perf_counter_ns -import os from typing import Callable, TypeVar, cast from typing_extensions import ParamSpec -from collections import deque from performance_metrics.datashapes import ( RawContextData, ) +from performance_metrics.data_store import MetricsStore from opentrons_shared_data.performance.dev_types import ( RobotContextState, SupportsTracking, + StorageMetadata, ) P = ParamSpec("P") @@ -47,14 +46,23 @@ def _get_timing_function() -> Callable[[], int]: class RobotContextTracker(SupportsTracking): """Tracks and stores robot context and execution duration for different operations.""" - FILE_NAME = "context_data.csv" + METADATA_NAME = "robot_context_data" def __init__(self, storage_location: Path, should_track: bool = False) -> None: """Initializes the RobotContextTracker with an empty storage list.""" - self._storage: deque[RawContextData] = deque() - self._storage_file_path = storage_location / self.FILE_NAME + self._store = MetricsStore[RawContextData]( + StorageMetadata( + name=self.METADATA_NAME, + storage_dir=storage_location, + storage_format="csv", + headers=RawContextData.headers(), + ) + ) self._should_track = should_track + if self._should_track: + self._store.setup() + def track(self, state: RobotContextState) -> Callable: # type: ignore """Decorator factory for tracking the execution duration and state of robot operations. @@ -77,14 +85,14 @@ def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: result = func(*args, **kwargs) finally: duration_end_time = perf_counter_ns() - self._storage.append( + self._store.data.append( RawContextData( - function_start_time, - duration_start_time, - duration_end_time, - state, + func_start=function_start_time, + duration=duration_end_time - duration_start_time, + state=state, ) ) + return result return wrapper @@ -93,11 +101,6 @@ def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: def store(self) -> None: """Returns the stored context data and clears the storage list.""" - stored_data = self._storage.copy() - self._storage.clear() - rows_to_write = [context_data.csv_row() for context_data in stored_data] - os.makedirs(self._storage_file_path.parent, exist_ok=True) - with open(self._storage_file_path, "a") as storage_file: - writer = csv.writer(storage_file) - writer.writerow(RawContextData.headers()) - writer.writerows(rows_to_write) + if not self._should_track: + return + self._store.store() diff --git a/performance-metrics/tests/performance_metrics/test_data_store.py b/performance-metrics/tests/performance_metrics/test_data_store.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py b/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py index 2c112410063..b49647c5b3e 100644 --- a/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py +++ b/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py @@ -2,6 +2,7 @@ import asyncio from pathlib import Path +from performance_metrics.datashapes import RawContextData import pytest from performance_metrics.robot_context_tracker import RobotContextTracker from opentrons_shared_data.performance.dev_types import RobotContextState @@ -47,7 +48,7 @@ def shutting_down_robot() -> None: # Ensure storage is initially empty assert ( - len(robot_context_tracker._storage) == 0 + len(robot_context_tracker._store.data) == 0 ), "Storage should be initially empty." starting_robot() @@ -57,7 +58,7 @@ def shutting_down_robot() -> None: shutting_down_robot() # Verify that all states were tracked - assert len(robot_context_tracker._storage) == 5, "All states should be tracked." + assert len(robot_context_tracker._store.data) == 5, "All states should be tracked." # Validate the sequence and accuracy of tracked states expected_states = [ @@ -69,7 +70,9 @@ def shutting_down_robot() -> None: ] for i, state in enumerate(expected_states): assert ( - RobotContextState.from_id(robot_context_tracker._storage[i].state.state_id) + RobotContextState.from_id( + robot_context_tracker._store.data[i].state.state_id + ) == state ), f"State at index {i} should be {state}." @@ -91,11 +94,11 @@ def second_operation() -> None: second_operation() assert ( - len(robot_context_tracker._storage) == 2 + len(robot_context_tracker._store.data) == 2 ), "Both operations should be tracked." assert ( - robot_context_tracker._storage[0].state - == robot_context_tracker._storage[1].state + robot_context_tracker._store.data[0].state + == robot_context_tracker._store.data[1].state == RobotContextState.RUNNING_PROTOCOL ), "Both operations should have the same state." @@ -114,10 +117,10 @@ def error_prone_operation() -> None: error_prone_operation() assert ( - len(robot_context_tracker._storage) == 1 + len(robot_context_tracker._store.data) == 1 ), "Failed operation should still be tracked." assert ( - robot_context_tracker._storage[0].state == RobotContextState.SHUTTING_DOWN + robot_context_tracker._store.data[0].state == RobotContextState.SHUTTING_DOWN ), "State should be correctly logged despite the exception." @@ -134,10 +137,11 @@ async def async_analyzing_operation() -> None: await async_analyzing_operation() assert ( - len(robot_context_tracker._storage) == 1 + len(robot_context_tracker._store.data) == 1 ), "Async operation should be tracked." assert ( - robot_context_tracker._storage[0].state == RobotContextState.ANALYZING_PROTOCOL + robot_context_tracker._store.data[0].state + == RobotContextState.ANALYZING_PROTOCOL ), "State should be ANALYZING_PROTOCOL." @@ -152,10 +156,9 @@ def running_operation() -> None: running_operation() - duration_data = robot_context_tracker._storage[0] - measured_duration = duration_data.duration_end - duration_data.duration_start + duration_data = robot_context_tracker._store.data[0] assert ( - abs(measured_duration - RUNNING_TIME * 1e9) < 1e7 + abs(duration_data.duration - RUNNING_TIME * 1e9) < 1e7 ), "Measured duration for sync operation should closely match the expected duration." @@ -171,10 +174,9 @@ async def async_running_operation() -> None: await async_running_operation() - duration_data = robot_context_tracker._storage[0] - measured_duration = duration_data.duration_end - duration_data.duration_start + duration_data = robot_context_tracker._store.data[0] assert ( - abs(measured_duration - RUNNING_TIME * 1e9) < 1e7 + abs(duration_data.duration - RUNNING_TIME * 1e9) < 1e7 ), "Measured duration for async operation should closely match the expected duration." @@ -193,10 +195,10 @@ async def async_error_prone_operation() -> None: await async_error_prone_operation() assert ( - len(robot_context_tracker._storage) == 1 + len(robot_context_tracker._store.data) == 1 ), "Failed async operation should still be tracked." assert ( - robot_context_tracker._storage[0].state == RobotContextState.SHUTTING_DOWN + robot_context_tracker._store.data[0].state == RobotContextState.SHUTTING_DOWN ), "State should be SHUTTING_DOWN despite the exception." @@ -217,11 +219,11 @@ async def second_async_calibrating() -> None: await asyncio.gather(first_async_calibrating(), second_async_calibrating()) assert ( - len(robot_context_tracker._storage) == 2 + len(robot_context_tracker._store.data) == 2 ), "Both concurrent async operations should be tracked." assert all( data.state == RobotContextState.CALIBRATING - for data in robot_context_tracker._storage + for data in robot_context_tracker._store.data ), "All tracked operations should be in CALIBRATING state." @@ -236,7 +238,7 @@ def operation_without_tracking() -> None: operation_without_tracking() assert ( - len(robot_context_tracker._storage) == 0 + len(robot_context_tracker._store.data) == 0 ), "Operation should not be tracked when tracking is disabled." @@ -262,11 +264,19 @@ def analyzing_protocol() -> None: robot_context_tracker.store() - with open(robot_context_tracker._storage_file_path, "r") as file: + with open(robot_context_tracker._store.metadata.data_file_location, "r") as file: lines = file.readlines() - assert ( - len(lines) == 4 - ), "All stored data + header should be written to the file." + assert len(lines) == 3, "All stored data should be written to the file." + + split_lines: list[list[str]] = [line.strip().split(",") for line in lines] + assert all( + RawContextData.from_csv_row(line) for line in split_lines + ), "All lines should be valid RawContextData instances." + + with open(robot_context_tracker._store.metadata.headers_file_location, "r") as file: + headers = file.readlines() + assert len(headers) == 1, "Header should be written to the headers file." + assert tuple(headers[0].strip().split(",")) == RawContextData.headers() @patch( @@ -289,17 +299,11 @@ def calibrating_robot() -> None: starting_robot() calibrating_robot() - storage = robot_context_tracker._storage + storage = robot_context_tracker._store.data assert all( data.func_start > 0 for data in storage ), "All function start times should be greater than 0." assert all( - data.duration_start > 0 for data in storage - ), "All duration start times should be greater than 0." - assert all( - data.duration_end > 0 for data in storage - ), "All duration end times should be greater than 0." - assert all( - data.duration_end > data.duration_start for data in storage - ), "Duration end times should be greater than duration start times." + data.duration > 0 for data in storage + ), "All duration times should be greater than 0." assert len(storage) == 2, "Both operations should be tracked." diff --git a/shared-data/python/opentrons_shared_data/performance/dev_types.py b/shared-data/python/opentrons_shared_data/performance/dev_types.py index 842399f2c3b..39bc897cad4 100644 --- a/shared-data/python/opentrons_shared_data/performance/dev_types.py +++ b/shared-data/python/opentrons_shared_data/performance/dev_types.py @@ -1,6 +1,6 @@ """Type definitions for performance tracking.""" - -from typing import Protocol, TypeVar, Callable, Any +from dataclasses import dataclass +from typing import Literal, Protocol, Tuple, TypeVar, Callable, Any from pathlib import Path from enum import Enum @@ -54,3 +54,25 @@ def from_id(cls, state_id: int) -> "RobotContextState": if state.state_id == state_id: return state raise ValueError(f"Invalid state id: {state_id}") + + +@dataclass(frozen=True) +class StorageMetadata: + """Dataclass to store information about the storage.""" + + name: str + storage_dir: Path + storage_format: Literal["csv"] + headers: Tuple[str, ...] + + @property + def data_file_location(self) -> Path: + """The location of the data file.""" + return self.storage_dir / self.name + + @property + def headers_file_location(self) -> Path: + """The location of the header file.""" + return self.data_file_location.with_stem( + self.data_file_location.stem + "_headers" + ) From e8ebdf204d16048f112ab212769b366fac485550 Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Tue, 23 Apr 2024 06:18:42 -0700 Subject: [PATCH 02/22] chore: add add method. Move data store tests --- .../src/performance_metrics/data_store.py | 10 ++- .../robot_context_tracker.py | 2 +- .../performance_metrics/test_data_store.py | 52 +++++++++++++ .../test_robot_context_tracker.py | 75 +++++-------------- 4 files changed, 79 insertions(+), 60 deletions(-) diff --git a/performance-metrics/src/performance_metrics/data_store.py b/performance-metrics/src/performance_metrics/data_store.py index b64ed4021e3..708ee57f257 100644 --- a/performance-metrics/src/performance_metrics/data_store.py +++ b/performance-metrics/src/performance_metrics/data_store.py @@ -14,7 +14,11 @@ class MetricsStore(typing.Generic[T]): def __init__(self, metadata: StorageMetadata) -> None: """Initialize the storage engine.""" self.metadata = metadata - self.data: typing.List[T] = [] + self._data: typing.List[T] = [] + + def add(self, context_data: T) -> None: + """Add data to the store.""" + self._data.append(context_data) def setup(self) -> None: """Set up the data store.""" @@ -25,8 +29,8 @@ def setup(self) -> None: def store(self) -> None: """Clear the stored data and write it to the storage file.""" - stored_data = self.data.copy() - self.data.clear() + stored_data = self._data.copy() + self._data.clear() rows_to_write = [context_data.csv_row() for context_data in stored_data] with open(self.metadata.data_file_location, "a") as storage_file: writer = csv.writer(storage_file) diff --git a/performance-metrics/src/performance_metrics/robot_context_tracker.py b/performance-metrics/src/performance_metrics/robot_context_tracker.py index 7e310126837..64c98c6dfa8 100644 --- a/performance-metrics/src/performance_metrics/robot_context_tracker.py +++ b/performance-metrics/src/performance_metrics/robot_context_tracker.py @@ -85,7 +85,7 @@ def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: result = func(*args, **kwargs) finally: duration_end_time = perf_counter_ns() - self._store.data.append( + self._store.add( RawContextData( func_start=function_start_time, duration=duration_end_time - duration_start_time, diff --git a/performance-metrics/tests/performance_metrics/test_data_store.py b/performance-metrics/tests/performance_metrics/test_data_store.py index e69de29bb2d..d9b795484dc 100644 --- a/performance-metrics/tests/performance_metrics/test_data_store.py +++ b/performance-metrics/tests/performance_metrics/test_data_store.py @@ -0,0 +1,52 @@ +"""Tests for the data store.""" + +from pathlib import Path +from time import sleep + +from opentrons_shared_data.performance.dev_types import RobotContextState +from performance_metrics.datashapes import RawContextData +from performance_metrics.robot_context_tracker import RobotContextTracker + +# Corrected times in seconds +STARTING_TIME = 0.001 +CALIBRATING_TIME = 0.002 +ANALYZING_TIME = 0.003 +RUNNING_TIME = 0.004 +SHUTTING_DOWN_TIME = 0.005 + + +async def test_storing_to_file(tmp_path: Path) -> None: + """Tests storing the tracked data to a file.""" + robot_context_tracker = RobotContextTracker(tmp_path, should_track=True) + + @robot_context_tracker.track(state=RobotContextState.STARTING_UP) + def starting_robot() -> None: + sleep(STARTING_TIME) + + @robot_context_tracker.track(state=RobotContextState.CALIBRATING) + def calibrating_robot() -> None: + sleep(CALIBRATING_TIME) + + @robot_context_tracker.track(state=RobotContextState.ANALYZING_PROTOCOL) + def analyzing_protocol() -> None: + sleep(ANALYZING_TIME) + + starting_robot() + calibrating_robot() + analyzing_protocol() + + robot_context_tracker.store() + + with open(robot_context_tracker._store.metadata.data_file_location, "r") as file: + lines = file.readlines() + assert len(lines) == 3, "All stored data should be written to the file." + + split_lines: list[list[str]] = [line.strip().split(",") for line in lines] + assert all( + RawContextData.from_csv_row(line) for line in split_lines + ), "All lines should be valid RawContextData instances." + + with open(robot_context_tracker._store.metadata.headers_file_location, "r") as file: + headers = file.readlines() + assert len(headers) == 1, "Header should be written to the headers file." + assert tuple(headers[0].strip().split(",")) == RawContextData.headers() diff --git a/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py b/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py index b49647c5b3e..9b642c8a0fd 100644 --- a/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py +++ b/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py @@ -48,7 +48,7 @@ def shutting_down_robot() -> None: # Ensure storage is initially empty assert ( - len(robot_context_tracker._store.data) == 0 + len(robot_context_tracker._store._data) == 0 ), "Storage should be initially empty." starting_robot() @@ -58,7 +58,7 @@ def shutting_down_robot() -> None: shutting_down_robot() # Verify that all states were tracked - assert len(robot_context_tracker._store.data) == 5, "All states should be tracked." + assert len(robot_context_tracker._store._data) == 5, "All states should be tracked." # Validate the sequence and accuracy of tracked states expected_states = [ @@ -71,7 +71,7 @@ def shutting_down_robot() -> None: for i, state in enumerate(expected_states): assert ( RobotContextState.from_id( - robot_context_tracker._store.data[i].state.state_id + robot_context_tracker._store._data[i].state.state_id ) == state ), f"State at index {i} should be {state}." @@ -94,11 +94,11 @@ def second_operation() -> None: second_operation() assert ( - len(robot_context_tracker._store.data) == 2 + len(robot_context_tracker._store._data) == 2 ), "Both operations should be tracked." assert ( - robot_context_tracker._store.data[0].state - == robot_context_tracker._store.data[1].state + robot_context_tracker._store._data[0].state + == robot_context_tracker._store._data[1].state == RobotContextState.RUNNING_PROTOCOL ), "Both operations should have the same state." @@ -117,10 +117,10 @@ def error_prone_operation() -> None: error_prone_operation() assert ( - len(robot_context_tracker._store.data) == 1 + len(robot_context_tracker._store._data) == 1 ), "Failed operation should still be tracked." assert ( - robot_context_tracker._store.data[0].state == RobotContextState.SHUTTING_DOWN + robot_context_tracker._store._data[0].state == RobotContextState.SHUTTING_DOWN ), "State should be correctly logged despite the exception." @@ -137,10 +137,10 @@ async def async_analyzing_operation() -> None: await async_analyzing_operation() assert ( - len(robot_context_tracker._store.data) == 1 + len(robot_context_tracker._store._data) == 1 ), "Async operation should be tracked." assert ( - robot_context_tracker._store.data[0].state + robot_context_tracker._store._data[0].state == RobotContextState.ANALYZING_PROTOCOL ), "State should be ANALYZING_PROTOCOL." @@ -156,7 +156,7 @@ def running_operation() -> None: running_operation() - duration_data = robot_context_tracker._store.data[0] + duration_data = robot_context_tracker._store._data[0] assert ( abs(duration_data.duration - RUNNING_TIME * 1e9) < 1e7 ), "Measured duration for sync operation should closely match the expected duration." @@ -174,7 +174,7 @@ async def async_running_operation() -> None: await async_running_operation() - duration_data = robot_context_tracker._store.data[0] + duration_data = robot_context_tracker._store._data[0] assert ( abs(duration_data.duration - RUNNING_TIME * 1e9) < 1e7 ), "Measured duration for async operation should closely match the expected duration." @@ -195,10 +195,10 @@ async def async_error_prone_operation() -> None: await async_error_prone_operation() assert ( - len(robot_context_tracker._store.data) == 1 + len(robot_context_tracker._store._data) == 1 ), "Failed async operation should still be tracked." assert ( - robot_context_tracker._store.data[0].state == RobotContextState.SHUTTING_DOWN + robot_context_tracker._store._data[0].state == RobotContextState.SHUTTING_DOWN ), "State should be SHUTTING_DOWN despite the exception." @@ -219,11 +219,11 @@ async def second_async_calibrating() -> None: await asyncio.gather(first_async_calibrating(), second_async_calibrating()) assert ( - len(robot_context_tracker._store.data) == 2 + len(robot_context_tracker._store._data) == 2 ), "Both concurrent async operations should be tracked." assert all( data.state == RobotContextState.CALIBRATING - for data in robot_context_tracker._store.data + for data in robot_context_tracker._store._data ), "All tracked operations should be in CALIBRATING state." @@ -238,46 +238,9 @@ def operation_without_tracking() -> None: operation_without_tracking() assert ( - len(robot_context_tracker._store.data) == 0 + len(robot_context_tracker._store._data) == 0 ), "Operation should not be tracked when tracking is disabled." - - -async def test_storing_to_file(tmp_path: Path) -> None: - """Tests storing the tracked data to a file.""" - robot_context_tracker = RobotContextTracker(tmp_path, should_track=True) - - @robot_context_tracker.track(state=RobotContextState.STARTING_UP) - def starting_robot() -> None: - sleep(STARTING_TIME) - - @robot_context_tracker.track(state=RobotContextState.CALIBRATING) - def calibrating_robot() -> None: - sleep(CALIBRATING_TIME) - - @robot_context_tracker.track(state=RobotContextState.ANALYZING_PROTOCOL) - def analyzing_protocol() -> None: - sleep(ANALYZING_TIME) - - starting_robot() - calibrating_robot() - analyzing_protocol() - - robot_context_tracker.store() - - with open(robot_context_tracker._store.metadata.data_file_location, "r") as file: - lines = file.readlines() - assert len(lines) == 3, "All stored data should be written to the file." - - split_lines: list[list[str]] = [line.strip().split(",") for line in lines] - assert all( - RawContextData.from_csv_row(line) for line in split_lines - ), "All lines should be valid RawContextData instances." - - with open(robot_context_tracker._store.metadata.headers_file_location, "r") as file: - headers = file.readlines() - assert len(headers) == 1, "Header should be written to the headers file." - assert tuple(headers[0].strip().split(",")) == RawContextData.headers() - + @patch( "performance_metrics.robot_context_tracker._get_timing_function", @@ -299,7 +262,7 @@ def calibrating_robot() -> None: starting_robot() calibrating_robot() - storage = robot_context_tracker._store.data + storage = robot_context_tracker._store._data assert all( data.func_start > 0 for data in storage ), "All function start times should be greater than 0." From 9ee562a671a810ead8c44fb25ae409493066045b Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Tue, 23 Apr 2024 06:21:04 -0700 Subject: [PATCH 03/22] chore: small fixes --- api/tests/opentrons/cli/test_cli.py | 4 ++-- .../tests/performance_metrics/test_robot_context_tracker.py | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/api/tests/opentrons/cli/test_cli.py b/api/tests/opentrons/cli/test_cli.py index 83d9468347e..32de5f3105e 100644 --- a/api/tests/opentrons/cli/test_cli.py +++ b/api/tests/opentrons/cli/test_cli.py @@ -285,11 +285,11 @@ def run(protocol): protocol_source_file.write_text(protocol_source, encoding="utf-8") store = context_tracker._store # type: ignore[attr-defined] - num_storage_entities_before_analysis = len(store.data) + num_storage_entities_before_analysis = len(store._data) _get_analysis_result([protocol_source_file]) - assert len(store.data) == num_storage_entities_before_analysis + 1 + assert len(store._data) == num_storage_entities_before_analysis + 1 with open(store.metadata.data_file_location, "r") as f: stored_data = f.readlines() diff --git a/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py b/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py index 9b642c8a0fd..1d26b87cb2e 100644 --- a/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py +++ b/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py @@ -2,7 +2,6 @@ import asyncio from pathlib import Path -from performance_metrics.datashapes import RawContextData import pytest from performance_metrics.robot_context_tracker import RobotContextTracker from opentrons_shared_data.performance.dev_types import RobotContextState @@ -240,7 +239,7 @@ def operation_without_tracking() -> None: assert ( len(robot_context_tracker._store._data) == 0 ), "Operation should not be tracked when tracking is disabled." - + @patch( "performance_metrics.robot_context_tracker._get_timing_function", From d8d6ca338883644d1bc4423941dff8e875660bbd Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Tue, 23 Apr 2024 06:29:08 -0700 Subject: [PATCH 04/22] Stop constant from being modified --- .../src/performance_metrics/robot_context_tracker.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/performance-metrics/src/performance_metrics/robot_context_tracker.py b/performance-metrics/src/performance_metrics/robot_context_tracker.py index 64c98c6dfa8..ae21ccf1e3f 100644 --- a/performance-metrics/src/performance_metrics/robot_context_tracker.py +++ b/performance-metrics/src/performance_metrics/robot_context_tracker.py @@ -5,7 +5,7 @@ from functools import wraps, partial from time import perf_counter_ns -from typing import Callable, TypeVar, cast +from typing import Callable, TypeVar, cast, Literal, Final from typing_extensions import ParamSpec @@ -46,7 +46,7 @@ def _get_timing_function() -> Callable[[], int]: class RobotContextTracker(SupportsTracking): """Tracks and stores robot context and execution duration for different operations.""" - METADATA_NAME = "robot_context_data" + METADATA_NAME: Final[Literal["robot_context_data"]] = "robot_context_data" def __init__(self, storage_location: Path, should_track: bool = False) -> None: """Initializes the RobotContextTracker with an empty storage list.""" From 6797acaf3da77fb9f2502b0891368332eb2fca9d Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Tue, 23 Apr 2024 06:40:36 -0700 Subject: [PATCH 05/22] storage format is not being used --- .../src/performance_metrics/robot_context_tracker.py | 1 - .../python/opentrons_shared_data/performance/dev_types.py | 1 - 2 files changed, 2 deletions(-) diff --git a/performance-metrics/src/performance_metrics/robot_context_tracker.py b/performance-metrics/src/performance_metrics/robot_context_tracker.py index ae21ccf1e3f..af2c5af8017 100644 --- a/performance-metrics/src/performance_metrics/robot_context_tracker.py +++ b/performance-metrics/src/performance_metrics/robot_context_tracker.py @@ -54,7 +54,6 @@ def __init__(self, storage_location: Path, should_track: bool = False) -> None: StorageMetadata( name=self.METADATA_NAME, storage_dir=storage_location, - storage_format="csv", headers=RawContextData.headers(), ) ) diff --git a/shared-data/python/opentrons_shared_data/performance/dev_types.py b/shared-data/python/opentrons_shared_data/performance/dev_types.py index 39bc897cad4..05cda39d3a1 100644 --- a/shared-data/python/opentrons_shared_data/performance/dev_types.py +++ b/shared-data/python/opentrons_shared_data/performance/dev_types.py @@ -62,7 +62,6 @@ class StorageMetadata: name: str storage_dir: Path - storage_format: Literal["csv"] headers: Tuple[str, ...] @property From b405cefd4431547300609d2b3d67207379070153 Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Tue, 23 Apr 2024 06:54:42 -0700 Subject: [PATCH 06/22] Make file name match --- .../src/performance_metrics/{data_store.py => metrics_store.py} | 2 +- .../src/performance_metrics/robot_context_tracker.py | 2 +- .../{test_data_store.py => test_metrics_store.py} | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) rename performance-metrics/src/performance_metrics/{data_store.py => metrics_store.py} (96%) rename performance-metrics/tests/performance_metrics/{test_data_store.py => test_metrics_store.py} (98%) diff --git a/performance-metrics/src/performance_metrics/data_store.py b/performance-metrics/src/performance_metrics/metrics_store.py similarity index 96% rename from performance-metrics/src/performance_metrics/data_store.py rename to performance-metrics/src/performance_metrics/metrics_store.py index 708ee57f257..fb78be9e41e 100644 --- a/performance-metrics/src/performance_metrics/data_store.py +++ b/performance-metrics/src/performance_metrics/metrics_store.py @@ -12,7 +12,7 @@ class MetricsStore(typing.Generic[T]): """Dataclass to store data for tracking robot context.""" def __init__(self, metadata: StorageMetadata) -> None: - """Initialize the storage engine.""" + """Initialize the metrics store.""" self.metadata = metadata self._data: typing.List[T] = [] diff --git a/performance-metrics/src/performance_metrics/robot_context_tracker.py b/performance-metrics/src/performance_metrics/robot_context_tracker.py index af2c5af8017..d45192046ea 100644 --- a/performance-metrics/src/performance_metrics/robot_context_tracker.py +++ b/performance-metrics/src/performance_metrics/robot_context_tracker.py @@ -12,7 +12,7 @@ from performance_metrics.datashapes import ( RawContextData, ) -from performance_metrics.data_store import MetricsStore +from performance_metrics.metrics_store import MetricsStore from opentrons_shared_data.performance.dev_types import ( RobotContextState, SupportsTracking, diff --git a/performance-metrics/tests/performance_metrics/test_data_store.py b/performance-metrics/tests/performance_metrics/test_metrics_store.py similarity index 98% rename from performance-metrics/tests/performance_metrics/test_data_store.py rename to performance-metrics/tests/performance_metrics/test_metrics_store.py index d9b795484dc..ea58afc6388 100644 --- a/performance-metrics/tests/performance_metrics/test_data_store.py +++ b/performance-metrics/tests/performance_metrics/test_metrics_store.py @@ -1,4 +1,4 @@ -"""Tests for the data store.""" +"""Tests for the metrics store.""" from pathlib import Path from time import sleep From e5a077915e21fd8e465601d08f29ad2697f0c289 Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Tue, 23 Apr 2024 06:56:28 -0700 Subject: [PATCH 07/22] another typo --- performance-metrics/src/performance_metrics/metrics_store.py | 4 ++-- .../src/performance_metrics/robot_context_tracker.py | 4 ++-- .../python/opentrons_shared_data/performance/dev_types.py | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/performance-metrics/src/performance_metrics/metrics_store.py b/performance-metrics/src/performance_metrics/metrics_store.py index fb78be9e41e..49793a34cae 100644 --- a/performance-metrics/src/performance_metrics/metrics_store.py +++ b/performance-metrics/src/performance_metrics/metrics_store.py @@ -2,7 +2,7 @@ import csv import typing -from opentrons_shared_data.performance.dev_types import StorageMetadata +from opentrons_shared_data.performance.dev_types import MetricsMetadata from performance_metrics.datashapes import SupportsCSVStorage T = typing.TypeVar("T", bound=SupportsCSVStorage) @@ -11,7 +11,7 @@ class MetricsStore(typing.Generic[T]): """Dataclass to store data for tracking robot context.""" - def __init__(self, metadata: StorageMetadata) -> None: + def __init__(self, metadata: MetricsMetadata) -> None: """Initialize the metrics store.""" self.metadata = metadata self._data: typing.List[T] = [] diff --git a/performance-metrics/src/performance_metrics/robot_context_tracker.py b/performance-metrics/src/performance_metrics/robot_context_tracker.py index d45192046ea..a6472bd8959 100644 --- a/performance-metrics/src/performance_metrics/robot_context_tracker.py +++ b/performance-metrics/src/performance_metrics/robot_context_tracker.py @@ -16,7 +16,7 @@ from opentrons_shared_data.performance.dev_types import ( RobotContextState, SupportsTracking, - StorageMetadata, + MetricsMetadata, ) P = ParamSpec("P") @@ -51,7 +51,7 @@ class RobotContextTracker(SupportsTracking): def __init__(self, storage_location: Path, should_track: bool = False) -> None: """Initializes the RobotContextTracker with an empty storage list.""" self._store = MetricsStore[RawContextData]( - StorageMetadata( + MetricsMetadata( name=self.METADATA_NAME, storage_dir=storage_location, headers=RawContextData.headers(), diff --git a/shared-data/python/opentrons_shared_data/performance/dev_types.py b/shared-data/python/opentrons_shared_data/performance/dev_types.py index 05cda39d3a1..ab01740431b 100644 --- a/shared-data/python/opentrons_shared_data/performance/dev_types.py +++ b/shared-data/python/opentrons_shared_data/performance/dev_types.py @@ -57,8 +57,8 @@ def from_id(cls, state_id: int) -> "RobotContextState": @dataclass(frozen=True) -class StorageMetadata: - """Dataclass to store information about the storage.""" +class MetricsMetadata: + """Dataclass to store metadata about the metrics.""" name: str storage_dir: Path From e7a4d28d1fa23b8da2007ca30ecf57648887eefc Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Tue, 23 Apr 2024 07:48:48 -0700 Subject: [PATCH 08/22] linting --- .../python/opentrons_shared_data/performance/dev_types.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shared-data/python/opentrons_shared_data/performance/dev_types.py b/shared-data/python/opentrons_shared_data/performance/dev_types.py index ab01740431b..15ddb5eba44 100644 --- a/shared-data/python/opentrons_shared_data/performance/dev_types.py +++ b/shared-data/python/opentrons_shared_data/performance/dev_types.py @@ -1,6 +1,6 @@ """Type definitions for performance tracking.""" from dataclasses import dataclass -from typing import Literal, Protocol, Tuple, TypeVar, Callable, Any +from typing import Protocol, Tuple, TypeVar, Callable, Any from pathlib import Path from enum import Enum From 97896047d24af9a7193573a75d3f6b0bbc06553b Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Tue, 23 Apr 2024 08:21:11 -0700 Subject: [PATCH 09/22] hopefully fix whitespace issue on windows --- api/tests/opentrons/cli/test_cli.py | 1 + 1 file changed, 1 insertion(+) diff --git a/api/tests/opentrons/cli/test_cli.py b/api/tests/opentrons/cli/test_cli.py index 32de5f3105e..e3450d8a12e 100644 --- a/api/tests/opentrons/cli/test_cli.py +++ b/api/tests/opentrons/cli/test_cli.py @@ -299,6 +299,7 @@ def run(protocol): with open(store.metadata.data_file_location, "r") as f: stored_data = f.readlines() + stored_data = [line.strip() for line in stored_data if line.strip()] assert len(stored_data) == 1 state_id, start_time, duration = stored_data[0].strip().split(",") assert state_id == str(RobotContextState.ANALYZING_PROTOCOL.state_id) From 700770db20eb6bf5455073eb047afcbcf632fa16 Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Tue, 23 Apr 2024 08:55:52 -0700 Subject: [PATCH 10/22] feat: Add storing analysis context on track --- api/src/opentrons/util/performance_helpers.py | 24 ++++++-- api/tests/opentrons/cli/test_cli.py | 59 +++++++++++++++---- 2 files changed, 68 insertions(+), 15 deletions(-) diff --git a/api/src/opentrons/util/performance_helpers.py b/api/src/opentrons/util/performance_helpers.py index a157908303d..acb7cfd4750 100644 --- a/api/src/opentrons/util/performance_helpers.py +++ b/api/src/opentrons/util/performance_helpers.py @@ -1,5 +1,6 @@ """Performance helpers for tracking robot context.""" +import functools from pathlib import Path from opentrons_shared_data.performance.dev_types import ( SupportsTracking, @@ -18,6 +19,7 @@ _should_track = ff.enable_performance_metrics( RobotTypeEnum.robot_literal_to_enum(robot_configs.load().model) ) +STORE_EACH = _should_track class StubbedTracker(SupportsTracking): @@ -70,7 +72,21 @@ def _get_robot_context_tracker() -> SupportsTracking: def track_analysis(func: F) -> F: - """Track the analysis of a protocol.""" - return _get_robot_context_tracker().track(RobotContextState.ANALYZING_PROTOCOL)( - func - ) + """Track the analysis of a protocol and optionally store each run.""" + # This will probably not stick around very long but it gives me + # the ability to test this on a robot + + # Typing a decorator that wraps a decorator with args, nope + @functools.wraps(func) + def wrapper(*args, **kwargs): # type: ignore # noqa: ANN002, ANN003, ANN201 + tracker = _get_robot_context_tracker() + tracked_func = tracker.track(RobotContextState.ANALYZING_PROTOCOL)(func) + + result = tracked_func(*args, **kwargs) + + if STORE_EACH: + tracker.store() + + return result + + return wrapper # type: ignore diff --git a/api/tests/opentrons/cli/test_cli.py b/api/tests/opentrons/cli/test_cli.py index e3450d8a12e..bb8722985d1 100644 --- a/api/tests/opentrons/cli/test_cli.py +++ b/api/tests/opentrons/cli/test_cli.py @@ -39,6 +39,26 @@ def override_data_store(tmp_path: Path) -> Iterator[None]: context_tracker._store = old_store # type: ignore[attr-defined] +@pytest.fixture +def monkeypatch_set_store_each_to_true(monkeypatch: pytest.MonkeyPatch) -> None: + """Override the STORE_EACH flag for the RobotContextTracker.""" + monkeypatch.setattr("opentrons.util.performance_helpers.STORE_EACH", True) + + +def verify_metrics_store_file(file_path: Path, expected_length: int) -> None: + """Verify that the metrics store file contains the expected number of lines.""" + with open(file_path, "r") as f: + stored_data = f.readlines() + stored_data = [line.strip() for line in stored_data if line.strip()] + assert len(stored_data) == expected_length + for line in stored_data: + state_id, start_time, duration = line.strip().split(",") + assert state_id.isdigit() + assert state_id == str(RobotContextState.ANALYZING_PROTOCOL.state_id) + assert start_time.isdigit() + assert duration.isdigit() + + def _list_fixtures(version: int) -> Iterator[Path]: return Path(__file__).parent.glob( f"../../../../shared-data/protocol/fixtures/{version}/*.json" @@ -291,17 +311,34 @@ def run(protocol): assert len(store._data) == num_storage_entities_before_analysis + 1 - with open(store.metadata.data_file_location, "r") as f: - stored_data = f.readlines() - assert len(stored_data) == 0 + verify_metrics_store_file(store.metadata.data_file_location, 0) context_tracker.store() - with open(store.metadata.data_file_location, "r") as f: - stored_data = f.readlines() - stored_data = [line.strip() for line in stored_data if line.strip()] - assert len(stored_data) == 1 - state_id, start_time, duration = stored_data[0].strip().split(",") - assert state_id == str(RobotContextState.ANALYZING_PROTOCOL.state_id) - assert start_time.isdigit() - assert duration.isdigit() + verify_metrics_store_file(store.metadata.data_file_location, 1) + + +@pytest.mark.usefixtures("override_data_store", "monkeypatch_set_store_each_to_true") +def test_track_analysis_with_store_each_run(tmp_path: Path) -> None: + """Test that the RobotContextTracker tracks analysis and stores each run.""" + protocol_source = textwrap.dedent( + """ + requirements = {"apiLevel": "2.15"} + + def run(protocol): + pass + """ + ) + protocol_source_file = tmp_path / "protocol.py" + protocol_source_file.write_text(protocol_source, encoding="utf-8") + store = context_tracker._store # type: ignore[attr-defined] + + verify_metrics_store_file(store.metadata.data_file_location, 0) + + _get_analysis_result([protocol_source_file]) + + verify_metrics_store_file(store.metadata.data_file_location, 1) + + _get_analysis_result([protocol_source_file]) + + verify_metrics_store_file(store.metadata.data_file_location, 2) From cb16dd74ab04cb9fbee9ae1abb129b0425ef9e27 Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Thu, 25 Apr 2024 08:09:39 -0700 Subject: [PATCH 11/22] WIP --- .gitignore | 4 + api/src/opentrons/__init__.py | 3 +- api/src/opentrons/util/logging_config.py | 4 + api/src/opentrons/util/performance_helpers.py | 14 ++- performance-metrics/.gitignore | 3 +- performance-metrics/Makefile | 92 ++++++++++++++++++- performance-metrics/pyproject.toml | 3 + performance-metrics/setup.cfg | 11 +++ scripts/python_build_utils.py | 1 + 9 files changed, 123 insertions(+), 12 deletions(-) create mode 100644 performance-metrics/pyproject.toml create mode 100755 performance-metrics/setup.cfg diff --git a/.gitignore b/.gitignore index e3f5d0620a8..847a80ff148 100755 --- a/.gitignore +++ b/.gitignore @@ -160,3 +160,7 @@ opentrons-robot-app.tar.gz # asdf versions file .tool-versions mock_dir + + +# +remote-debugging/ \ No newline at end of file diff --git a/api/src/opentrons/__init__.py b/api/src/opentrons/__init__.py index ac4e0c54262..b24a8a52820 100755 --- a/api/src/opentrons/__init__.py +++ b/api/src/opentrons/__init__.py @@ -145,7 +145,8 @@ async def initialize() -> ThreadManagedHardware: """ robot_conf = robot_configs.load() logging_config.log_init(robot_conf.log_level) - + log.info(f"Logging Level: {robot_conf.log_level}") + log.info(f"Performance Metrics Feature Flag: {ff.enable_performance_metrics(robot_conf.model)}") log.info(f"API server version: {version}") log.info(f"Robot Name: {name()}") diff --git a/api/src/opentrons/util/logging_config.py b/api/src/opentrons/util/logging_config.py index e9a4d2042a2..112f4c7594f 100644 --- a/api/src/opentrons/util/logging_config.py +++ b/api/src/opentrons/util/logging_config.py @@ -66,6 +66,10 @@ def _host_config(level_value: int) -> Dict[str, Any]: "level": logging.DEBUG, "propagate": False, }, + "opentrons.util.performance_helpers":{ + "handlers": ["debug", "api"], + "level": level_value, + }, "__main__": {"handlers": ["api"], "level": level_value}, }, } diff --git a/api/src/opentrons/util/performance_helpers.py b/api/src/opentrons/util/performance_helpers.py index acb7cfd4750..296f2d084c5 100644 --- a/api/src/opentrons/util/performance_helpers.py +++ b/api/src/opentrons/util/performance_helpers.py @@ -1,6 +1,7 @@ """Performance helpers for tracking robot context.""" import functools +import logging from pathlib import Path from opentrons_shared_data.performance.dev_types import ( SupportsTracking, @@ -10,17 +11,14 @@ from opentrons_shared_data.robot.dev_types import RobotTypeEnum from typing import Callable, Type from opentrons.config import ( - feature_flags as ff, get_performance_metrics_data_dir, - robot_configs, ) +log = logging.getLogger(__name__) -_should_track = ff.enable_performance_metrics( - RobotTypeEnum.robot_literal_to_enum(robot_configs.load().model) -) -STORE_EACH = _should_track +_should_track = True +STORE_EACH = _should_track class StubbedTracker(SupportsTracking): """A stubbed tracker that does nothing.""" @@ -50,7 +48,6 @@ def _handle_package_import() -> Type[SupportsTracking]: """ try: from performance_metrics import RobotContextTracker - return RobotContextTracker except ImportError: return StubbedTracker @@ -63,8 +60,8 @@ def _handle_package_import() -> Type[SupportsTracking]: def _get_robot_context_tracker() -> SupportsTracking: """Singleton for the robot context tracker.""" global _robot_context_tracker + log.error(f"Using performance metrics: {_should_track}") if _robot_context_tracker is None: - # TODO: replace with path lookup and should_store lookup _robot_context_tracker = package_to_use( get_performance_metrics_data_dir(), _should_track ) @@ -79,6 +76,7 @@ def track_analysis(func: F) -> F: # Typing a decorator that wraps a decorator with args, nope @functools.wraps(func) def wrapper(*args, **kwargs): # type: ignore # noqa: ANN002, ANN003, ANN201 + log.error("Tracking analysis") tracker = _get_robot_context_tracker() tracked_func = tracker.track(RobotContextState.ANALYZING_PROTOCOL)(func) diff --git a/performance-metrics/.gitignore b/performance-metrics/.gitignore index 8fb3d9a4ea5..9bddb5fd873 100644 --- a/performance-metrics/.gitignore +++ b/performance-metrics/.gitignore @@ -1 +1,2 @@ -.ruff_cache/ \ No newline at end of file +.ruff_cache/ +.logs/ \ No newline at end of file diff --git a/performance-metrics/Makefile b/performance-metrics/Makefile index fd4dd421ad2..9487e6cb4d7 100644 --- a/performance-metrics/Makefile +++ b/performance-metrics/Makefile @@ -1,4 +1,35 @@ include ../scripts/python.mk +include ../scripts/push.mk + +ot_project := $(OPENTRONS_PROJECT) +project_rs_default = $(if $(ot_project),$(ot_project),robot-stack) +project_ir_default = $(if $(ot_project),$(ot_project),ot3) + +SHX := npx shx + +# Host key location for robot +ssh_key ?= $(default_ssh_key) +# Other SSH args for robot +ssh_opts ?= $(default_ssh_opts) +# Helper to safely bundle ssh options +ssh_helper = $(if $(ssh_key),-i $(ssh_key)) $(ssh_opts) + +# Defined separately than the clean target so the wheel file doesn’t have to +# depend on a PHONY target + +# Find the version of the wheel from git using a helper script. We +# use python here so we can use the same version normalization that will be +# used to create the wheel. +wheel_file = dist/$(call python_get_wheelname,performance-metrics,$(project_rs_default),performance_metrics,$(BUILD_NUMBER)) + +# Find the version of the sdist file from git using a helper script. +sdist_file = dist/$(call python_get_sdistname,performance_metrics,$(project_rs_default),performance_metrics) + + +clean_cmd = $(SHX) rm -rf 'build' '**/*.egg-info' '**/__pycache__' **/*.pyc '.mypy_cache' '.pytest_cache' +clean_wheel_cmd = $(clean_cmd) dist/*.whl +clean_sdist_cmd = $(clean_cmd) dist/*.tar.gz +clean_all_cmd = $(clean_cmd) dist .PHONY: lint lint: @@ -20,12 +51,69 @@ teardown: .PHONY: clean clean: - rm -rf build dist *.egg-info .mypy_cache .pytest_cache src/performance_metrics.egg-info + $(clean_all_cmd) .PHONY: wheel wheel: + $(clean_wheel_cmd) $(python) setup.py $(wheel_opts) bdist_wheel - rm -rf build + $(SHX) rm -rf build + $(SHX) ls dist + +.PHONY: sdist +sdist: export OPENTRONS_PROJECT=$(project_rs_default) +sdist: + $(clean_sdist_cmd) + $(python) setup.py sdist + $(SHX) rm -rf build + $(SHX) ls dist + +.PHONY: push-no-restart +push-no-restart: wheel + $(call push-python-package,$(host),$(ssh_key),$(ssh_opts),$(wheel_file)) + +.PHONY: push +push: push-no-restart + $(call restart-service,$(host),$(ssh_key),$(ssh_opts),"opentrons-robot-server") + +.PHONY: enable-ff-on-robot +enable-ff-on-robot: + @echo "Enabling performance metrics on robot" + curl \ + -X POST\ + -H "opentrons-version: *" \ + -H "Content-Type: application/json" \ + -d '{"id": "enablePerformanceMetrics", "value": true}' \ + http://$(host):31950/settings >> /dev/null + +.PHONY: disable-ff-on-robot +disable-ff-on-robot: + @echo "Disabling performance metrics on robot" + curl \ + -X POST\ + -H "opentrons-version: *" \ + -H "Content-Type: application/json" \ + -d '{"id": "enablePerformanceMetrics", "value": false}' \ + http://$(host):31950/settings | jq + +.PHONY: set-debug-log-level +set-debug-log-level: + @echo "Setting log level to debug on robot" + curl \ + -X POST\ + -H "opentrons-version: *" \ + -H "Content-Type: application/json" \ + -d '{"log_level": "debug"}' \ + http://$(host):31950/settings/log_level/local | jq + +.PHONY: get-logs +get-logs: + @echo "Creating logs directory" + mkdir -p .logs + + @echo "Getting logs from robot" + ssh -i $(ssh_key) $(ssh_opts) root@$(host) "journalctl --no-pager --boot --catalog --unit opentrons-robot-server.service" > .logs/robot-server.log + .PHONY: test test: diff --git a/performance-metrics/pyproject.toml b/performance-metrics/pyproject.toml new file mode 100644 index 00000000000..b0471b7f6a8 --- /dev/null +++ b/performance-metrics/pyproject.toml @@ -0,0 +1,3 @@ +[build-system] +requires = ["setuptools", "wheel"] +build-backend = "setuptools.build_meta:__legacy__" \ No newline at end of file diff --git a/performance-metrics/setup.cfg b/performance-metrics/setup.cfg new file mode 100755 index 00000000000..5f426ffc147 --- /dev/null +++ b/performance-metrics/setup.cfg @@ -0,0 +1,11 @@ +[bdist_wheel] +universal = 1 + +[metadata] +license_files = ../LICENSE + +[pep8] +ignore = E221,E222 + +[aliases] +test=pytest diff --git a/scripts/python_build_utils.py b/scripts/python_build_utils.py index d55ece0e0c8..9949e2ced71 100644 --- a/scripts/python_build_utils.py +++ b/scripts/python_build_utils.py @@ -32,6 +32,7 @@ 'usb-bridge': PackageEntry('usb_bridge'), 'system-server': PackageEntry('system_server'), 'server-utils': PackageEntry('server_utils'), + 'performance-metrics': PackageEntry('performance_metrics'), } project_entries = { From eccf62a8ee97659af7f501600ab13dab83e35799 Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Thu, 25 Apr 2024 13:15:47 -0700 Subject: [PATCH 12/22] fix: move store_each functionality to robot_context_tracker --- api/src/opentrons/util/performance_helpers.py | 15 +++---- .../robot_context_tracker.py | 10 +++-- .../tests/performance_metrics/conftest.py | 14 ++++++ .../performance_metrics/test_metrics_store.py | 2 +- .../test_robot_context_tracker.py | 44 +++++++++++++++++-- .../performance/dev_types.py | 2 +- 6 files changed, 70 insertions(+), 17 deletions(-) create mode 100644 performance-metrics/tests/performance_metrics/conftest.py diff --git a/api/src/opentrons/util/performance_helpers.py b/api/src/opentrons/util/performance_helpers.py index 296f2d084c5..1411a320dde 100644 --- a/api/src/opentrons/util/performance_helpers.py +++ b/api/src/opentrons/util/performance_helpers.py @@ -12,18 +12,20 @@ from typing import Callable, Type from opentrons.config import ( get_performance_metrics_data_dir, + robot_configs, ) -log = logging.getLogger(__name__) -_should_track = True +_should_track = ff.enable_performance_metrics( + RobotTypeEnum.robot_literal_to_enum(robot_configs.load().model) +) STORE_EACH = _should_track class StubbedTracker(SupportsTracking): """A stubbed tracker that does nothing.""" - def __init__(self, storage_location: Path, should_track: bool) -> None: + def __init__(self, storage_location: Path, should_track: bool, store_each: bool) -> None: """Initialize the stubbed tracker.""" pass @@ -60,10 +62,9 @@ def _handle_package_import() -> Type[SupportsTracking]: def _get_robot_context_tracker() -> SupportsTracking: """Singleton for the robot context tracker.""" global _robot_context_tracker - log.error(f"Using performance metrics: {_should_track}") if _robot_context_tracker is None: _robot_context_tracker = package_to_use( - get_performance_metrics_data_dir(), _should_track + get_performance_metrics_data_dir(), _should_track, STORE_EACH ) return _robot_context_tracker @@ -76,15 +77,11 @@ def track_analysis(func: F) -> F: # Typing a decorator that wraps a decorator with args, nope @functools.wraps(func) def wrapper(*args, **kwargs): # type: ignore # noqa: ANN002, ANN003, ANN201 - log.error("Tracking analysis") tracker = _get_robot_context_tracker() tracked_func = tracker.track(RobotContextState.ANALYZING_PROTOCOL)(func) result = tracked_func(*args, **kwargs) - if STORE_EACH: - tracker.store() - return result return wrapper # type: ignore diff --git a/performance-metrics/src/performance_metrics/robot_context_tracker.py b/performance-metrics/src/performance_metrics/robot_context_tracker.py index a6472bd8959..9cf4eb1891d 100644 --- a/performance-metrics/src/performance_metrics/robot_context_tracker.py +++ b/performance-metrics/src/performance_metrics/robot_context_tracker.py @@ -48,7 +48,7 @@ class RobotContextTracker(SupportsTracking): METADATA_NAME: Final[Literal["robot_context_data"]] = "robot_context_data" - def __init__(self, storage_location: Path, should_track: bool = False) -> None: + def __init__(self, storage_location: Path, should_track: bool, store_each: bool) -> None: """Initializes the RobotContextTracker with an empty storage list.""" self._store = MetricsStore[RawContextData]( MetricsMetadata( @@ -58,6 +58,7 @@ def __init__(self, storage_location: Path, should_track: bool = False) -> None: ) ) self._should_track = should_track + self._store_each = store_each if self._should_track: self._store.setup() @@ -89,10 +90,13 @@ def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: func_start=function_start_time, duration=duration_end_time - duration_start_time, state=state, + ) ) - ) - return result + if self._store_each: + self.store() + + return result return wrapper diff --git a/performance-metrics/tests/performance_metrics/conftest.py b/performance-metrics/tests/performance_metrics/conftest.py new file mode 100644 index 00000000000..adbe0fed16d --- /dev/null +++ b/performance-metrics/tests/performance_metrics/conftest.py @@ -0,0 +1,14 @@ +from pathlib import Path + + +def verify_metrics_store_file(file_path: Path, expected_length: int) -> None: + """Verify that the metrics store file contains the expected number of lines.""" + with open(file_path, "r") as f: + stored_data = f.readlines() + stored_data = [line.strip() for line in stored_data if line.strip()] + assert len(stored_data) == expected_length + for line in stored_data: + state_id, start_time, duration = line.strip().split(",") + assert state_id.isdigit() + assert start_time.isdigit() + assert duration.isdigit() \ No newline at end of file diff --git a/performance-metrics/tests/performance_metrics/test_metrics_store.py b/performance-metrics/tests/performance_metrics/test_metrics_store.py index ea58afc6388..31079170a98 100644 --- a/performance-metrics/tests/performance_metrics/test_metrics_store.py +++ b/performance-metrics/tests/performance_metrics/test_metrics_store.py @@ -17,7 +17,7 @@ async def test_storing_to_file(tmp_path: Path) -> None: """Tests storing the tracked data to a file.""" - robot_context_tracker = RobotContextTracker(tmp_path, should_track=True) + robot_context_tracker = RobotContextTracker(tmp_path, should_track=True, store_each=False) @robot_context_tracker.track(state=RobotContextState.STARTING_UP) def starting_robot() -> None: diff --git a/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py b/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py index 1d26b87cb2e..332a2a7a58b 100644 --- a/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py +++ b/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py @@ -7,6 +7,7 @@ from opentrons_shared_data.performance.dev_types import RobotContextState from time import sleep, time_ns from unittest.mock import patch +from conftest import verify_metrics_store_file # Corrected times in seconds STARTING_TIME = 0.001 @@ -19,7 +20,7 @@ @pytest.fixture def robot_context_tracker(tmp_path: Path) -> RobotContextTracker: """Fixture to provide a fresh instance of RobotContextTracker for each test.""" - return RobotContextTracker(storage_location=tmp_path, should_track=True) + return RobotContextTracker(storage_location=tmp_path, should_track=True, store_each=False) def test_robot_context_tracker(robot_context_tracker: RobotContextTracker) -> None: @@ -228,7 +229,7 @@ async def second_async_calibrating() -> None: def test_no_tracking(tmp_path: Path) -> None: """Tests that operations are not tracked when tracking is disabled.""" - robot_context_tracker = RobotContextTracker(tmp_path, should_track=False) + robot_context_tracker = RobotContextTracker(tmp_path, should_track=False, store_each=False) @robot_context_tracker.track(state=RobotContextState.STARTING_UP) def operation_without_tracking() -> None: @@ -248,7 +249,7 @@ def operation_without_tracking() -> None: def test_using_non_linux_time_functions(tmp_path: Path) -> None: """Tests tracking operations using non-Linux time functions.""" file_path = tmp_path / "test_file.csv" - robot_context_tracker = RobotContextTracker(file_path, should_track=True) + robot_context_tracker = RobotContextTracker(file_path, should_track=True, store_each=False) @robot_context_tracker.track(state=RobotContextState.STARTING_UP) def starting_robot() -> None: @@ -269,3 +270,40 @@ def calibrating_robot() -> None: data.duration > 0 for data in storage ), "All duration times should be greater than 0." assert len(storage) == 2, "Both operations should be tracked." + + +def test_store_each(tmp_path: Path) -> None: + """Tests storing data after each operation when store_each is enabled.""" + + robot_context_tracker = RobotContextTracker(tmp_path, should_track=True, store_each=True) + + @robot_context_tracker.track(state=RobotContextState.STARTING_UP) + def starting_robot() -> None: + sleep(STARTING_TIME) + + + @robot_context_tracker.track(state=RobotContextState.CALIBRATING) + def calibrating_robot() -> None: + sleep(CALIBRATING_TIME) + + starting_robot() + + assert ( + len(robot_context_tracker._store._data) == 0 + ), ( + "Operation should automatically stored when store_each is enabled." + "So _data should be cleared after every track" + ) + verify_metrics_store_file(robot_context_tracker._store.metadata.data_file_location, 1) + + + calibrating_robot() + + assert ( + len(robot_context_tracker._store._data) == 0 + ), ( + "Operation should automatically stored when store_each is enabled." + "So _data should be cleared after every track" + ) + + verify_metrics_store_file(robot_context_tracker._store.metadata.data_file_location, 2) \ No newline at end of file diff --git a/shared-data/python/opentrons_shared_data/performance/dev_types.py b/shared-data/python/opentrons_shared_data/performance/dev_types.py index 15ddb5eba44..33b30a08ed9 100644 --- a/shared-data/python/opentrons_shared_data/performance/dev_types.py +++ b/shared-data/python/opentrons_shared_data/performance/dev_types.py @@ -10,7 +10,7 @@ class SupportsTracking(Protocol): """Protocol for classes that support tracking of robot context.""" - def __init__(self, storage_location: Path, should_track: bool) -> None: + def __init__(self, storage_location: Path, should_track: bool, store_each: bool) -> None: """Initialize the tracker.""" ... From 416d3b23f8abd2e86762dfffc3a391e9084d7f88 Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Thu, 25 Apr 2024 13:16:39 -0700 Subject: [PATCH 13/22] fix: turns out async functions were not being handled at all --- .../robot_context_tracker.py | 50 ++++++++++++++----- 1 file changed, 37 insertions(+), 13 deletions(-) diff --git a/performance-metrics/src/performance_metrics/robot_context_tracker.py b/performance-metrics/src/performance_metrics/robot_context_tracker.py index 9cf4eb1891d..d72c1352aab 100644 --- a/performance-metrics/src/performance_metrics/robot_context_tracker.py +++ b/performance-metrics/src/performance_metrics/robot_context_tracker.py @@ -1,5 +1,6 @@ """Module for tracking robot context and execution duration for different operations.""" +import inspect from pathlib import Path import platform @@ -77,19 +78,42 @@ def inner_decorator(func: Callable[P, R]) -> Callable[P, R]: if not self._should_track: return func - @wraps(func) - def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: - function_start_time = timing_function() - duration_start_time = perf_counter_ns() - try: - result = func(*args, **kwargs) - finally: - duration_end_time = perf_counter_ns() - self._store.add( - RawContextData( - func_start=function_start_time, - duration=duration_end_time - duration_start_time, - state=state, + if inspect.iscoroutinefunction(func): + @wraps(func) + async def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: + function_start_time = timing_function() + duration_start_time = perf_counter_ns() + try: + result = await func(*args, **kwargs) + finally: + duration_end_time = perf_counter_ns() + + self._store.add( + RawContextData( + func_start=function_start_time, + duration=duration_end_time - duration_start_time, + state=state, + ) + ) + + if self._store_each: + self.store() + + return result + else: + @wraps(func) + def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: + function_start_time = timing_function() + duration_start_time = perf_counter_ns() + try: + result = func(*args, **kwargs) + finally: + duration_end_time = perf_counter_ns() + self._store.add( + RawContextData( + func_start=function_start_time, + duration=duration_end_time - duration_start_time, + state=state, ) ) From 995ef45b5f4c33951b9b04f521ff09a26240bf13 Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Thu, 25 Apr 2024 13:17:11 -0700 Subject: [PATCH 14/22] test: only test store each functionality --- api/tests/opentrons/cli/test_cli.py | 30 +---------------------------- 1 file changed, 1 insertion(+), 29 deletions(-) diff --git a/api/tests/opentrons/cli/test_cli.py b/api/tests/opentrons/cli/test_cli.py index bb8722985d1..f74f6361531 100644 --- a/api/tests/opentrons/cli/test_cli.py +++ b/api/tests/opentrons/cli/test_cli.py @@ -42,7 +42,7 @@ def override_data_store(tmp_path: Path) -> Iterator[None]: @pytest.fixture def monkeypatch_set_store_each_to_true(monkeypatch: pytest.MonkeyPatch) -> None: """Override the STORE_EACH flag for the RobotContextTracker.""" - monkeypatch.setattr("opentrons.util.performance_helpers.STORE_EACH", True) + context_tracker._store_each = True # type: ignore[attr-defined] def verify_metrics_store_file(file_path: Path, expected_length: int) -> None: @@ -290,34 +290,6 @@ def test_python_error_line_numbers( assert error["detail"] == expected_detail -@pytest.mark.usefixtures("override_data_store") -def test_track_analysis(tmp_path: Path) -> None: - """Test that the RobotContextTracker tracks analysis.""" - protocol_source = textwrap.dedent( - """ - requirements = {"apiLevel": "2.15"} - - def run(protocol): - pass - """ - ) - protocol_source_file = tmp_path / "protocol.py" - protocol_source_file.write_text(protocol_source, encoding="utf-8") - store = context_tracker._store # type: ignore[attr-defined] - - num_storage_entities_before_analysis = len(store._data) - - _get_analysis_result([protocol_source_file]) - - assert len(store._data) == num_storage_entities_before_analysis + 1 - - verify_metrics_store_file(store.metadata.data_file_location, 0) - - context_tracker.store() - - verify_metrics_store_file(store.metadata.data_file_location, 1) - - @pytest.mark.usefixtures("override_data_store", "monkeypatch_set_store_each_to_true") def test_track_analysis_with_store_each_run(tmp_path: Path) -> None: """Test that the RobotContextTracker tracks analysis and stores each run.""" From 9eca61e3843c31b5f8016a511a5af7efd3d2c017 Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Thu, 25 Apr 2024 13:17:37 -0700 Subject: [PATCH 15/22] chore: helps if I actually wrap the correct function --- .../protocols/protocol_analyzer.py | 4 +- .../tests/protocols/test_protocol_analyzer.py | 60 ++++++++++++++++++- 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/robot-server/robot_server/protocols/protocol_analyzer.py b/robot-server/robot_server/protocols/protocol_analyzer.py index 1ecee27e1da..5ab3dccdfa8 100644 --- a/robot-server/robot_server/protocols/protocol_analyzer.py +++ b/robot-server/robot_server/protocols/protocol_analyzer.py @@ -5,6 +5,7 @@ from opentrons import protocol_runner from opentrons.protocol_engine.errors import ErrorOccurrence from opentrons.protocol_engine.types import RunTimeParamValuesType +from opentrons.util.performance_helpers import track_analysis import opentrons.util.helpers as datetime_helper import robot_server.errors.error_mappers as em @@ -24,7 +25,8 @@ def __init__( ) -> None: """Initialize the analyzer and its dependencies.""" self._analysis_store = analysis_store - + + @track_analysis async def analyze( self, protocol_resource: ProtocolResource, diff --git a/robot-server/tests/protocols/test_protocol_analyzer.py b/robot-server/tests/protocols/test_protocol_analyzer.py index c0c63e9e34f..8b4297868f2 100644 --- a/robot-server/tests/protocols/test_protocol_analyzer.py +++ b/robot-server/tests/protocols/test_protocol_analyzer.py @@ -1,4 +1,6 @@ """Tests for the ProtocolAnalyzer.""" +from dataclasses import replace +from typing import Iterator import pytest from decoy import Decoy from datetime import datetime @@ -26,6 +28,48 @@ from opentrons_shared_data.errors import EnumeratedError, ErrorCodes +from opentrons_shared_data.performance.dev_types import ( + RobotContextState, +) +from opentrons.util.performance_helpers import _get_robot_context_tracker + +# Enable tracking for the RobotContextTracker +# This must come before the import of the analyze CLI +context_tracker = _get_robot_context_tracker() + +# Ignore the type error for the next line, as we're setting a private attribute for testing purposes +context_tracker._should_track = True # type: ignore[attr-defined] + +@pytest.fixture +def override_data_store(tmp_path: Path) -> Iterator[None]: + """Override the data store metadata for the RobotContextTracker.""" + old_store = context_tracker._store # type: ignore[attr-defined] + old_metadata = old_store.metadata + new_metadata = replace(old_metadata, storage_dir=tmp_path) + context_tracker._store = old_store.__class__(metadata=new_metadata) # type: ignore[attr-defined] + context_tracker._store.setup() # type: ignore[attr-defined] + yield + context_tracker._store = old_store # type: ignore[attr-defined] + + +@pytest.fixture +def monkeypatch_set_store_each_to_true(monkeypatch: pytest.MonkeyPatch) -> None: + """Override the STORE_EACH flag for the RobotContextTracker.""" + context_tracker._store_each = True # type: ignore[attr-defined] + +def verify_metrics_store_file(file_path: Path, expected_length: int) -> None: + """Verify that the metrics store file contains the expected number of lines.""" + with open(file_path, "r") as f: + stored_data = f.readlines() + stored_data = [line.strip() for line in stored_data if line.strip()] + assert len(stored_data) == expected_length + for line in stored_data: + state_id, start_time, duration = line.strip().split(",") + assert state_id.isdigit() + assert state_id == str(RobotContextState.ANALYZING_PROTOCOL.state_id) + assert start_time.isdigit() + assert duration.isdigit() + @pytest.fixture(autouse=True) def patch_mock_map_unexpected_error( @@ -67,7 +111,7 @@ def subject( analysis_store=analysis_store, ) - +@pytest.mark.usefixtures("override_data_store", "monkeypatch_set_store_each_to_true") async def test_analyze( decoy: Decoy, analysis_store: AnalysisStore, @@ -160,12 +204,18 @@ async def test_analyze( ) ) + store = context_tracker._store # type: ignore[attr-defined] + + verify_metrics_store_file(store.metadata.data_file_location, 0) + await subject.analyze( protocol_resource=protocol_resource, analysis_id="analysis-id", run_time_param_values=None, ) + verify_metrics_store_file(store.metadata.data_file_location, 1) + decoy.verify( await analysis_store.update( analysis_id="analysis-id", @@ -180,6 +230,14 @@ async def test_analyze( ), ) + await subject.analyze( + protocol_resource=protocol_resource, + analysis_id="analysis-id", + run_time_param_values=None, + ) + + verify_metrics_store_file(store.metadata.data_file_location, 2) + async def test_analyze_updates_pending_on_error( decoy: Decoy, From f8bdbf0d9fd30eeba90ba1c9ba47f2e7a53bc928 Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Thu, 25 Apr 2024 13:26:36 -0700 Subject: [PATCH 16/22] chore: formatting and linting --- api/src/opentrons/__init__.py | 1 - api/src/opentrons/util/logging_config.py | 4 --- api/src/opentrons/util/performance_helpers.py | 9 +++-- .../util/test_performance_helpers.py | 2 +- .../robot_context_tracker.py | 11 ++++-- .../tests/performance_metrics/conftest.py | 4 ++- .../performance_metrics/test_metrics_store.py | 4 ++- .../test_robot_context_tracker.py | 35 +++++++++++-------- .../protocols/protocol_analyzer.py | 2 +- .../tests/protocols/test_protocol_analyzer.py | 3 ++ 10 files changed, 45 insertions(+), 30 deletions(-) diff --git a/api/src/opentrons/__init__.py b/api/src/opentrons/__init__.py index b24a8a52820..7e90a00a00f 100755 --- a/api/src/opentrons/__init__.py +++ b/api/src/opentrons/__init__.py @@ -146,7 +146,6 @@ async def initialize() -> ThreadManagedHardware: robot_conf = robot_configs.load() logging_config.log_init(robot_conf.log_level) log.info(f"Logging Level: {robot_conf.log_level}") - log.info(f"Performance Metrics Feature Flag: {ff.enable_performance_metrics(robot_conf.model)}") log.info(f"API server version: {version}") log.info(f"Robot Name: {name()}") diff --git a/api/src/opentrons/util/logging_config.py b/api/src/opentrons/util/logging_config.py index 112f4c7594f..e9a4d2042a2 100644 --- a/api/src/opentrons/util/logging_config.py +++ b/api/src/opentrons/util/logging_config.py @@ -66,10 +66,6 @@ def _host_config(level_value: int) -> Dict[str, Any]: "level": logging.DEBUG, "propagate": False, }, - "opentrons.util.performance_helpers":{ - "handlers": ["debug", "api"], - "level": level_value, - }, "__main__": {"handlers": ["api"], "level": level_value}, }, } diff --git a/api/src/opentrons/util/performance_helpers.py b/api/src/opentrons/util/performance_helpers.py index 1411a320dde..07904380c8a 100644 --- a/api/src/opentrons/util/performance_helpers.py +++ b/api/src/opentrons/util/performance_helpers.py @@ -1,7 +1,6 @@ """Performance helpers for tracking robot context.""" import functools -import logging from pathlib import Path from opentrons_shared_data.performance.dev_types import ( SupportsTracking, @@ -13,19 +12,22 @@ from opentrons.config import ( get_performance_metrics_data_dir, robot_configs, + feature_flags as ff, ) - _should_track = ff.enable_performance_metrics( RobotTypeEnum.robot_literal_to_enum(robot_configs.load().model) ) STORE_EACH = _should_track + class StubbedTracker(SupportsTracking): """A stubbed tracker that does nothing.""" - def __init__(self, storage_location: Path, should_track: bool, store_each: bool) -> None: + def __init__( + self, storage_location: Path, should_track: bool, store_each: bool + ) -> None: """Initialize the stubbed tracker.""" pass @@ -50,6 +52,7 @@ def _handle_package_import() -> Type[SupportsTracking]: """ try: from performance_metrics import RobotContextTracker + return RobotContextTracker except ImportError: return StubbedTracker diff --git a/api/tests/opentrons/util/test_performance_helpers.py b/api/tests/opentrons/util/test_performance_helpers.py index 57a42ef6a71..e3f8f04aef8 100644 --- a/api/tests/opentrons/util/test_performance_helpers.py +++ b/api/tests/opentrons/util/test_performance_helpers.py @@ -10,7 +10,7 @@ def test_return_function_unchanged() -> None: """Test that the function is returned unchanged when using StubbedTracker.""" - tracker = StubbedTracker(Path("/path/to/storage"), True) + tracker = StubbedTracker(Path("/path/to/storage"), True, store_each=True) def func_to_track() -> None: pass diff --git a/performance-metrics/src/performance_metrics/robot_context_tracker.py b/performance-metrics/src/performance_metrics/robot_context_tracker.py index d72c1352aab..494defd7930 100644 --- a/performance-metrics/src/performance_metrics/robot_context_tracker.py +++ b/performance-metrics/src/performance_metrics/robot_context_tracker.py @@ -49,7 +49,9 @@ class RobotContextTracker(SupportsTracking): METADATA_NAME: Final[Literal["robot_context_data"]] = "robot_context_data" - def __init__(self, storage_location: Path, should_track: bool, store_each: bool) -> None: + def __init__( + self, storage_location: Path, should_track: bool, store_each: bool + ) -> None: """Initializes the RobotContextTracker with an empty storage list.""" self._store = MetricsStore[RawContextData]( MetricsMetadata( @@ -79,6 +81,7 @@ def inner_decorator(func: Callable[P, R]) -> Callable[P, R]: return func if inspect.iscoroutinefunction(func): + @wraps(func) async def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: function_start_time = timing_function() @@ -99,8 +102,10 @@ async def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: if self._store_each: self.store() - return result + return result # type: ignore + else: + @wraps(func) def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: function_start_time = timing_function() @@ -122,7 +127,7 @@ def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: return result - return wrapper + return wrapper # type: ignore return inner_decorator diff --git a/performance-metrics/tests/performance_metrics/conftest.py b/performance-metrics/tests/performance_metrics/conftest.py index adbe0fed16d..9506321918c 100644 --- a/performance-metrics/tests/performance_metrics/conftest.py +++ b/performance-metrics/tests/performance_metrics/conftest.py @@ -1,3 +1,5 @@ +"""Shared fixtures and functions for performance metrics tests.""" + from pathlib import Path @@ -11,4 +13,4 @@ def verify_metrics_store_file(file_path: Path, expected_length: int) -> None: state_id, start_time, duration = line.strip().split(",") assert state_id.isdigit() assert start_time.isdigit() - assert duration.isdigit() \ No newline at end of file + assert duration.isdigit() diff --git a/performance-metrics/tests/performance_metrics/test_metrics_store.py b/performance-metrics/tests/performance_metrics/test_metrics_store.py index 31079170a98..76cbbb2a58d 100644 --- a/performance-metrics/tests/performance_metrics/test_metrics_store.py +++ b/performance-metrics/tests/performance_metrics/test_metrics_store.py @@ -17,7 +17,9 @@ async def test_storing_to_file(tmp_path: Path) -> None: """Tests storing the tracked data to a file.""" - robot_context_tracker = RobotContextTracker(tmp_path, should_track=True, store_each=False) + robot_context_tracker = RobotContextTracker( + tmp_path, should_track=True, store_each=False + ) @robot_context_tracker.track(state=RobotContextState.STARTING_UP) def starting_robot() -> None: diff --git a/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py b/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py index 332a2a7a58b..b61e2a01016 100644 --- a/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py +++ b/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py @@ -20,7 +20,9 @@ @pytest.fixture def robot_context_tracker(tmp_path: Path) -> RobotContextTracker: """Fixture to provide a fresh instance of RobotContextTracker for each test.""" - return RobotContextTracker(storage_location=tmp_path, should_track=True, store_each=False) + return RobotContextTracker( + storage_location=tmp_path, should_track=True, store_each=False + ) def test_robot_context_tracker(robot_context_tracker: RobotContextTracker) -> None: @@ -229,7 +231,9 @@ async def second_async_calibrating() -> None: def test_no_tracking(tmp_path: Path) -> None: """Tests that operations are not tracked when tracking is disabled.""" - robot_context_tracker = RobotContextTracker(tmp_path, should_track=False, store_each=False) + robot_context_tracker = RobotContextTracker( + tmp_path, should_track=False, store_each=False + ) @robot_context_tracker.track(state=RobotContextState.STARTING_UP) def operation_without_tracking() -> None: @@ -249,7 +253,9 @@ def operation_without_tracking() -> None: def test_using_non_linux_time_functions(tmp_path: Path) -> None: """Tests tracking operations using non-Linux time functions.""" file_path = tmp_path / "test_file.csv" - robot_context_tracker = RobotContextTracker(file_path, should_track=True, store_each=False) + robot_context_tracker = RobotContextTracker( + file_path, should_track=True, store_each=False + ) @robot_context_tracker.track(state=RobotContextState.STARTING_UP) def starting_robot() -> None: @@ -274,36 +280,35 @@ def calibrating_robot() -> None: def test_store_each(tmp_path: Path) -> None: """Tests storing data after each operation when store_each is enabled.""" - - robot_context_tracker = RobotContextTracker(tmp_path, should_track=True, store_each=True) + robot_context_tracker = RobotContextTracker( + tmp_path, should_track=True, store_each=True + ) @robot_context_tracker.track(state=RobotContextState.STARTING_UP) def starting_robot() -> None: sleep(STARTING_TIME) - @robot_context_tracker.track(state=RobotContextState.CALIBRATING) def calibrating_robot() -> None: sleep(CALIBRATING_TIME) starting_robot() - assert ( - len(robot_context_tracker._store._data) == 0 - ), ( + assert len(robot_context_tracker._store._data) == 0, ( "Operation should automatically stored when store_each is enabled." "So _data should be cleared after every track" ) - verify_metrics_store_file(robot_context_tracker._store.metadata.data_file_location, 1) - + verify_metrics_store_file( + robot_context_tracker._store.metadata.data_file_location, 1 + ) calibrating_robot() - assert ( - len(robot_context_tracker._store._data) == 0 - ), ( + assert len(robot_context_tracker._store._data) == 0, ( "Operation should automatically stored when store_each is enabled." "So _data should be cleared after every track" ) - verify_metrics_store_file(robot_context_tracker._store.metadata.data_file_location, 2) \ No newline at end of file + verify_metrics_store_file( + robot_context_tracker._store.metadata.data_file_location, 2 + ) diff --git a/robot-server/robot_server/protocols/protocol_analyzer.py b/robot-server/robot_server/protocols/protocol_analyzer.py index 5ab3dccdfa8..732cd840429 100644 --- a/robot-server/robot_server/protocols/protocol_analyzer.py +++ b/robot-server/robot_server/protocols/protocol_analyzer.py @@ -25,7 +25,7 @@ def __init__( ) -> None: """Initialize the analyzer and its dependencies.""" self._analysis_store = analysis_store - + @track_analysis async def analyze( self, diff --git a/robot-server/tests/protocols/test_protocol_analyzer.py b/robot-server/tests/protocols/test_protocol_analyzer.py index 8b4297868f2..fadace2f062 100644 --- a/robot-server/tests/protocols/test_protocol_analyzer.py +++ b/robot-server/tests/protocols/test_protocol_analyzer.py @@ -40,6 +40,7 @@ # Ignore the type error for the next line, as we're setting a private attribute for testing purposes context_tracker._should_track = True # type: ignore[attr-defined] + @pytest.fixture def override_data_store(tmp_path: Path) -> Iterator[None]: """Override the data store metadata for the RobotContextTracker.""" @@ -57,6 +58,7 @@ def monkeypatch_set_store_each_to_true(monkeypatch: pytest.MonkeyPatch) -> None: """Override the STORE_EACH flag for the RobotContextTracker.""" context_tracker._store_each = True # type: ignore[attr-defined] + def verify_metrics_store_file(file_path: Path, expected_length: int) -> None: """Verify that the metrics store file contains the expected number of lines.""" with open(file_path, "r") as f: @@ -111,6 +113,7 @@ def subject( analysis_store=analysis_store, ) + @pytest.mark.usefixtures("override_data_store", "monkeypatch_set_store_each_to_true") async def test_analyze( decoy: Decoy, From f1b122c97e02fe9a2ec7bd1db91be15c186009c2 Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Thu, 25 Apr 2024 13:28:24 -0700 Subject: [PATCH 17/22] missed this --- .gitignore | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index 847a80ff148..91bb3cb7d20 100755 --- a/.gitignore +++ b/.gitignore @@ -162,5 +162,4 @@ opentrons-robot-app.tar.gz mock_dir -# -remote-debugging/ \ No newline at end of file +# \ No newline at end of file From 4497565ede4fe860be3585a89020a4917f9a4c37 Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Fri, 26 Apr 2024 08:16:21 -0700 Subject: [PATCH 18/22] Add some makefile changes --- performance-metrics/Makefile | 41 +++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/performance-metrics/Makefile b/performance-metrics/Makefile index 9487e6cb4d7..a354b1ce659 100644 --- a/performance-metrics/Makefile +++ b/performance-metrics/Makefile @@ -76,25 +76,13 @@ push-no-restart: wheel push: push-no-restart $(call restart-service,$(host),$(ssh_key),$(ssh_opts),"opentrons-robot-server") -.PHONY: enable-ff-on-robot -enable-ff-on-robot: - @echo "Enabling performance metrics on robot" - curl \ - -X POST\ - -H "opentrons-version: *" \ - -H "Content-Type: application/json" \ - -d '{"id": "enablePerformanceMetrics", "value": true}' \ - http://$(host):31950/settings >> /dev/null +.PHONY: push-no-restart-ot3 +push-no-restart-ot3: sdist + $(call push-python-sdist,$(host),$(ssh_key),$(ssh_opts),$(sdist_file),"/opt/opentrons-robot-server","performance_metrics",,,$(version_file)) -.PHONY: disable-ff-on-robot -disable-ff-on-robot: - @echo "Disabling performance metrics on robot" - curl \ - -X POST\ - -H "opentrons-version: *" \ - -H "Content-Type: application/json" \ - -d '{"id": "enablePerformanceMetrics", "value": false}' \ - http://$(host):31950/settings | jq +.PHONY: push-ot3 +push-ot3: push-no-restart-ot3 + $(call restart-server,$(host),$(ssh_key),$(ssh_opts),"opentrons-robot-server") .PHONY: set-debug-log-level set-debug-log-level: @@ -106,13 +94,28 @@ set-debug-log-level: -d '{"log_level": "debug"}' \ http://$(host):31950/settings/log_level/local | jq +.PHONY: add-performance-metrics-ff +add-performance-metrics-ff: + @echo "Adding performance metrics feature flag to robot" + ssh -i $(ssh_key) $(ssh_opts) root@$(host) "touch /data/robot.env && \ + grep -q 'OT_API_FF_enablePerformanceMetrics' /data/robot.env && \ + sed -i 's/OT_API_FF_enablePerformanceMetrics=false/OT_API_FF_enablePerformanceMetrics=true/' /data/robot.env || \ + echo 'OT_API_FF_enablePerformanceMetrics=true' \ + >> /data/robot.env" + + .PHONY: get-logs get-logs: @echo "Creating logs directory" mkdir -p .logs @echo "Getting logs from robot" - ssh -i $(ssh_key) $(ssh_opts) root@$(host) "journalctl --no-pager --boot --catalog --unit opentrons-robot-server.service" > .logs/robot-server.log + ssh -i $(ssh_key) $(ssh_opts) root@$(host) "journalctl \ + --no-pager \ + --boot \ + --catalog \ + --unit opentrons-robot-server.service" \ + > .logs/robot-server.log .PHONY: test From 472cc362a6e17932cafd78036c299a33e2758f43 Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Fri, 26 Apr 2024 08:17:54 -0700 Subject: [PATCH 19/22] I just want the .gitignore to the be the same --- .gitignore | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/.gitignore b/.gitignore index 91bb3cb7d20..8d8277b56b4 100755 --- a/.gitignore +++ b/.gitignore @@ -159,7 +159,4 @@ opentrons-robot-app.tar.gz # asdf versions file .tool-versions -mock_dir - - -# \ No newline at end of file +mock_dir \ No newline at end of file From d69333740d0f5a0e23e8699fd64fb462fd1a7fd0 Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Fri, 26 Apr 2024 08:42:34 -0700 Subject: [PATCH 20/22] formatting --- .../python/opentrons_shared_data/performance/dev_types.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/shared-data/python/opentrons_shared_data/performance/dev_types.py b/shared-data/python/opentrons_shared_data/performance/dev_types.py index 33b30a08ed9..07d6c4dce8c 100644 --- a/shared-data/python/opentrons_shared_data/performance/dev_types.py +++ b/shared-data/python/opentrons_shared_data/performance/dev_types.py @@ -10,7 +10,9 @@ class SupportsTracking(Protocol): """Protocol for classes that support tracking of robot context.""" - def __init__(self, storage_location: Path, should_track: bool, store_each: bool) -> None: + def __init__( + self, storage_location: Path, should_track: bool, store_each: bool + ) -> None: """Initialize the tracker.""" ... From cb27ed1847851b5d68adc61e9128b2be9a0f4349 Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Fri, 26 Apr 2024 10:47:59 -0700 Subject: [PATCH 21/22] chore: bad conflict resolution skills --- api/tests/opentrons/cli/test_cli.py | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/api/tests/opentrons/cli/test_cli.py b/api/tests/opentrons/cli/test_cli.py index c9789f85e28..3e8d398998d 100644 --- a/api/tests/opentrons/cli/test_cli.py +++ b/api/tests/opentrons/cli/test_cli.py @@ -20,16 +20,6 @@ # Enable tracking for the RobotContextTracker # This must come before the import of the analyze CLI context_tracker = _get_robot_context_tracker() -@pytest.mark.usefixtures("override_data_store") -def test_track_analysis(tmp_path: Path) -> None: - """Test that the RobotContextTracker tracks analysis.""" -… requirements = {"apiLevel": "2.15"} - - def run(protocol): - pass - """ - ) - protocol_source_file = tmp_path / "protocol.py" # Ignore the type error for the next line, as we're setting a private attribute for testing purposes context_tracker._should_track = True # type: ignore[attr-defined] @@ -52,7 +42,7 @@ def override_data_store(tmp_path: Path) -> Iterator[None]: @pytest.fixture def monkeypatch_set_store_each_to_true(monkeypatch: pytest.MonkeyPatch) -> None: """Override the STORE_EACH flag for the RobotContextTracker.""" - context_tracker._store_each = True # type: ignore[attr-defined] + monkeypatch.setattr(context_tracker, "_store_each", True) def verify_metrics_store_file(file_path: Path, expected_length: int) -> None: From c85c295a7dbb68093bba8fc85e9400b59da2d84b Mon Sep 17 00:00:00 2001 From: Derek Maggio Date: Fri, 26 Apr 2024 13:33:03 -0700 Subject: [PATCH 22/22] For review --- api/src/opentrons/util/performance_helpers.py | 19 ++-- api/tests/opentrons/cli/test_cli.py | 12 +-- .../util/test_performance_helpers.py | 2 +- .../robot_context_tracker.py | 90 +++++++------------ .../performance_metrics/test_metrics_store.py | 4 +- .../test_robot_context_tracker.py | 49 +--------- .../performance/dev_types.py | 2 +- 7 files changed, 49 insertions(+), 129 deletions(-) diff --git a/api/src/opentrons/util/performance_helpers.py b/api/src/opentrons/util/performance_helpers.py index 07904380c8a..0096ba24163 100644 --- a/api/src/opentrons/util/performance_helpers.py +++ b/api/src/opentrons/util/performance_helpers.py @@ -1,5 +1,6 @@ """Performance helpers for tracking robot context.""" +import asyncio import functools from pathlib import Path from opentrons_shared_data.performance.dev_types import ( @@ -19,14 +20,12 @@ _should_track = ff.enable_performance_metrics( RobotTypeEnum.robot_literal_to_enum(robot_configs.load().model) ) -STORE_EACH = _should_track - class StubbedTracker(SupportsTracking): """A stubbed tracker that does nothing.""" def __init__( - self, storage_location: Path, should_track: bool, store_each: bool + self, storage_location: Path, should_track: bool ) -> None: """Initialize the stubbed tracker.""" pass @@ -67,7 +66,7 @@ def _get_robot_context_tracker() -> SupportsTracking: global _robot_context_tracker if _robot_context_tracker is None: _robot_context_tracker = package_to_use( - get_performance_metrics_data_dir(), _should_track, STORE_EACH + get_performance_metrics_data_dir(), _should_track ) return _robot_context_tracker @@ -79,12 +78,12 @@ def track_analysis(func: F) -> F: # Typing a decorator that wraps a decorator with args, nope @functools.wraps(func) - def wrapper(*args, **kwargs): # type: ignore # noqa: ANN002, ANN003, ANN201 - tracker = _get_robot_context_tracker() - tracked_func = tracker.track(RobotContextState.ANALYZING_PROTOCOL)(func) - - result = tracked_func(*args, **kwargs) + async def wrapper(*args, **kwargs): # type: ignore # noqa: ANN002, ANN003, ANN201 + tracker: SupportsTracking = _get_robot_context_tracker() - return result + try: + return await tracker.track(func, RobotContextState.ANALYZING_PROTOCOL, *args, **kwargs) + finally: + tracker.store() return wrapper # type: ignore diff --git a/api/tests/opentrons/cli/test_cli.py b/api/tests/opentrons/cli/test_cli.py index 3e8d398998d..668b2ffa1e9 100644 --- a/api/tests/opentrons/cli/test_cli.py +++ b/api/tests/opentrons/cli/test_cli.py @@ -39,12 +39,6 @@ def override_data_store(tmp_path: Path) -> Iterator[None]: context_tracker._store = old_store # type: ignore[attr-defined] -@pytest.fixture -def monkeypatch_set_store_each_to_true(monkeypatch: pytest.MonkeyPatch) -> None: - """Override the STORE_EACH flag for the RobotContextTracker.""" - monkeypatch.setattr(context_tracker, "_store_each", True) - - def verify_metrics_store_file(file_path: Path, expected_length: int) -> None: """Verify that the metrics store file contains the expected number of lines.""" with open(file_path, "r") as f: @@ -290,9 +284,9 @@ def test_python_error_line_numbers( assert error["detail"] == expected_detail -@pytest.mark.usefixtures("override_data_store", "monkeypatch_set_store_each_to_true") -def test_track_analysis_with_store_each_run(tmp_path: Path) -> None: - """Test that the RobotContextTracker tracks analysis and stores each run.""" +@pytest.mark.usefixtures("override_data_store") +def test_track_analysis_with_store(tmp_path: Path) -> None: + """Test that the RobotContextTracker tracks analysis and stores each execution.""" protocol_source = textwrap.dedent( """ requirements = {"apiLevel": "2.15"} diff --git a/api/tests/opentrons/util/test_performance_helpers.py b/api/tests/opentrons/util/test_performance_helpers.py index e3f8f04aef8..57a42ef6a71 100644 --- a/api/tests/opentrons/util/test_performance_helpers.py +++ b/api/tests/opentrons/util/test_performance_helpers.py @@ -10,7 +10,7 @@ def test_return_function_unchanged() -> None: """Test that the function is returned unchanged when using StubbedTracker.""" - tracker = StubbedTracker(Path("/path/to/storage"), True, store_each=True) + tracker = StubbedTracker(Path("/path/to/storage"), True) def func_to_track() -> None: pass diff --git a/performance-metrics/src/performance_metrics/robot_context_tracker.py b/performance-metrics/src/performance_metrics/robot_context_tracker.py index 494defd7930..23d3b2b3c16 100644 --- a/performance-metrics/src/performance_metrics/robot_context_tracker.py +++ b/performance-metrics/src/performance_metrics/robot_context_tracker.py @@ -49,9 +49,7 @@ class RobotContextTracker(SupportsTracking): METADATA_NAME: Final[Literal["robot_context_data"]] = "robot_context_data" - def __init__( - self, storage_location: Path, should_track: bool, store_each: bool - ) -> None: + def __init__(self, storage_location: Path, should_track: bool) -> None: """Initializes the RobotContextTracker with an empty storage list.""" self._store = MetricsStore[RawContextData]( MetricsMetadata( @@ -61,12 +59,11 @@ def __init__( ) ) self._should_track = should_track - self._store_each = store_each if self._should_track: self._store.setup() - def track(self, state: RobotContextState) -> Callable: # type: ignore + async def track(self, func_to_track: Callable, state: RobotContextState, *args, **kwargs) -> Callable: # type: ignore """Decorator factory for tracking the execution duration and state of robot operations. Args: @@ -76,60 +73,35 @@ def track(self, state: RobotContextState) -> Callable: # type: ignore Callable: A decorator that wraps a function to track its execution duration and state. """ - def inner_decorator(func: Callable[P, R]) -> Callable[P, R]: - if not self._should_track: - return func - - if inspect.iscoroutinefunction(func): - - @wraps(func) - async def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: - function_start_time = timing_function() - duration_start_time = perf_counter_ns() - try: - result = await func(*args, **kwargs) - finally: - duration_end_time = perf_counter_ns() - - self._store.add( - RawContextData( - func_start=function_start_time, - duration=duration_end_time - duration_start_time, - state=state, - ) - ) - - if self._store_each: - self.store() - - return result # type: ignore - - else: - - @wraps(func) - def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: - function_start_time = timing_function() - duration_start_time = perf_counter_ns() - try: - result = func(*args, **kwargs) - finally: - duration_end_time = perf_counter_ns() - self._store.add( - RawContextData( - func_start=function_start_time, - duration=duration_end_time - duration_start_time, - state=state, - ) - ) - - if self._store_each: - self.store() - - return result - - return wrapper # type: ignore - - return inner_decorator + + + if not self._should_track: + return func_to_track + + function_start_time = timing_function() + duration_start_time = perf_counter_ns() + + if inspect.iscoroutinefunction(func_to_track): + try: + result = await func_to_track(*args, **kwargs) + finally: + duration_end_time = perf_counter_ns() + else: + try: + result = func_to_track(*args, **kwargs) + finally: + duration_end_time = perf_counter_ns() + + self._store.add( + RawContextData( + func_start=function_start_time, + duration=duration_end_time - duration_start_time, + state=state, + ) + ) + return result + + def store(self) -> None: """Returns the stored context data and clears the storage list.""" diff --git a/performance-metrics/tests/performance_metrics/test_metrics_store.py b/performance-metrics/tests/performance_metrics/test_metrics_store.py index 76cbbb2a58d..ea58afc6388 100644 --- a/performance-metrics/tests/performance_metrics/test_metrics_store.py +++ b/performance-metrics/tests/performance_metrics/test_metrics_store.py @@ -17,9 +17,7 @@ async def test_storing_to_file(tmp_path: Path) -> None: """Tests storing the tracked data to a file.""" - robot_context_tracker = RobotContextTracker( - tmp_path, should_track=True, store_each=False - ) + robot_context_tracker = RobotContextTracker(tmp_path, should_track=True) @robot_context_tracker.track(state=RobotContextState.STARTING_UP) def starting_robot() -> None: diff --git a/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py b/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py index b61e2a01016..1d26b87cb2e 100644 --- a/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py +++ b/performance-metrics/tests/performance_metrics/test_robot_context_tracker.py @@ -7,7 +7,6 @@ from opentrons_shared_data.performance.dev_types import RobotContextState from time import sleep, time_ns from unittest.mock import patch -from conftest import verify_metrics_store_file # Corrected times in seconds STARTING_TIME = 0.001 @@ -20,9 +19,7 @@ @pytest.fixture def robot_context_tracker(tmp_path: Path) -> RobotContextTracker: """Fixture to provide a fresh instance of RobotContextTracker for each test.""" - return RobotContextTracker( - storage_location=tmp_path, should_track=True, store_each=False - ) + return RobotContextTracker(storage_location=tmp_path, should_track=True) def test_robot_context_tracker(robot_context_tracker: RobotContextTracker) -> None: @@ -231,9 +228,7 @@ async def second_async_calibrating() -> None: def test_no_tracking(tmp_path: Path) -> None: """Tests that operations are not tracked when tracking is disabled.""" - robot_context_tracker = RobotContextTracker( - tmp_path, should_track=False, store_each=False - ) + robot_context_tracker = RobotContextTracker(tmp_path, should_track=False) @robot_context_tracker.track(state=RobotContextState.STARTING_UP) def operation_without_tracking() -> None: @@ -253,9 +248,7 @@ def operation_without_tracking() -> None: def test_using_non_linux_time_functions(tmp_path: Path) -> None: """Tests tracking operations using non-Linux time functions.""" file_path = tmp_path / "test_file.csv" - robot_context_tracker = RobotContextTracker( - file_path, should_track=True, store_each=False - ) + robot_context_tracker = RobotContextTracker(file_path, should_track=True) @robot_context_tracker.track(state=RobotContextState.STARTING_UP) def starting_robot() -> None: @@ -276,39 +269,3 @@ def calibrating_robot() -> None: data.duration > 0 for data in storage ), "All duration times should be greater than 0." assert len(storage) == 2, "Both operations should be tracked." - - -def test_store_each(tmp_path: Path) -> None: - """Tests storing data after each operation when store_each is enabled.""" - robot_context_tracker = RobotContextTracker( - tmp_path, should_track=True, store_each=True - ) - - @robot_context_tracker.track(state=RobotContextState.STARTING_UP) - def starting_robot() -> None: - sleep(STARTING_TIME) - - @robot_context_tracker.track(state=RobotContextState.CALIBRATING) - def calibrating_robot() -> None: - sleep(CALIBRATING_TIME) - - starting_robot() - - assert len(robot_context_tracker._store._data) == 0, ( - "Operation should automatically stored when store_each is enabled." - "So _data should be cleared after every track" - ) - verify_metrics_store_file( - robot_context_tracker._store.metadata.data_file_location, 1 - ) - - calibrating_robot() - - assert len(robot_context_tracker._store._data) == 0, ( - "Operation should automatically stored when store_each is enabled." - "So _data should be cleared after every track" - ) - - verify_metrics_store_file( - robot_context_tracker._store.metadata.data_file_location, 2 - ) diff --git a/shared-data/python/opentrons_shared_data/performance/dev_types.py b/shared-data/python/opentrons_shared_data/performance/dev_types.py index 07d6c4dce8c..3b6de302e89 100644 --- a/shared-data/python/opentrons_shared_data/performance/dev_types.py +++ b/shared-data/python/opentrons_shared_data/performance/dev_types.py @@ -11,7 +11,7 @@ class SupportsTracking(Protocol): """Protocol for classes that support tracking of robot context.""" def __init__( - self, storage_location: Path, should_track: bool, store_each: bool + self, storage_location: Path, should_track: bool ) -> None: """Initialize the tracker.""" ...