Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(api): handle performance-metrics package not existing #14922

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/Pipfile
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,4 @@ pytest-profiling = "~=1.7.0"
# TODO(mc, 2022-03-31): upgrade sphinx, remove this subdep pin
jinja2 = ">=2.3,<3.1"
hypothesis = "==6.96.1"
performance-metrics = {file = "../performance-metrics", editable = true}
1,027 changes: 636 additions & 391 deletions api/Pipfile.lock

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions api/src/opentrons/cli/analyze.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
)

from opentrons_shared_data.robot.dev_types import RobotType
from opentrons.util.performance_helpers import track_analysis


@click.command()
Expand Down Expand Up @@ -63,6 +64,7 @@ def _get_input_files(files_and_dirs: Sequence[Path]) -> List[Path]:
return results


@track_analysis
async def _analyze(
files_and_dirs: Sequence[Path],
json_output: Optional[AsyncPath],
Expand Down
11 changes: 11 additions & 0 deletions api/src/opentrons/config/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,13 @@ class ConfigElement(NamedTuple):
ConfigElementType.DIR,
"The dir where module calibration is stored",
),
ConfigElement(
"performance_metrics_dir",
"Performance Metrics Directory",
Path("performance_metrics_data"),
ConfigElementType.DIR,
"The dir where performance metrics are stored",
),
)
#: The available configuration file elements to modify. All of these can be
#: changed by editing opentrons.json, where the keys are the name elements,
Expand Down Expand Up @@ -602,3 +609,7 @@ def get_tip_length_cal_path() -> Path:

def get_custom_tiprack_def_path() -> Path:
return get_opentrons_path("custom_tiprack_dir")


def get_performance_metrics_data_dir() -> Path:
return get_opentrons_path("performance_metrics_dir")
76 changes: 76 additions & 0 deletions api/src/opentrons/util/performance_helpers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
"""Performance helpers for tracking robot context."""

from pathlib import Path
from opentrons_shared_data.performance.dev_types import (
SupportsTracking,
F,
RobotContextState,
)
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,
)


_should_track = ff.enable_performance_metrics(
RobotTypeEnum.robot_literal_to_enum(robot_configs.load().model)
)


def _handle_package_import() -> Type[SupportsTracking]:
"""Handle the import of the performance_metrics package.

If the package is not available, return a stubbed tracker.
"""
try:
from performance_metrics import RobotContextTracker

return RobotContextTracker
except ImportError:
return StubbedTracker
Comment on lines +28 to +33
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I check opentrons.config.IS_ROBOT here?

if IS_ROBOT:
    try:
       from performance_metrics import RobotContextTracker
    except 
        <raise or log here>
else:
    return StubbedTracker

Copy link
Member

Choose a reason for hiding this comment

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

nah I think this is totally fine



package_to_use = _handle_package_import()
_robot_context_tracker: SupportsTracking | None = None


class StubbedTracker(SupportsTracking):
"""A stubbed tracker that does nothing."""

def __init__(self, storage_location: Path, should_track: bool) -> None:
"""Initialize the stubbed tracker."""
pass

def track(self, state: RobotContextState) -> Callable[[F], F]:
"""Return the function unchanged."""

def inner_decorator(func: F) -> F:
"""Return the function unchanged."""
return func

return inner_decorator

def store(self) -> None:
"""Do nothing."""
pass


def _get_robot_context_tracker() -> SupportsTracking:
"""Singleton for the robot context tracker."""
global _robot_context_tracker
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
)
return _robot_context_tracker


def track_analysis(func: F) -> F:
"""Track the analysis of a protocol."""
return _get_robot_context_tracker().track(RobotContextState.ANALYZING_PROTOCOL)(
func
)
34 changes: 33 additions & 1 deletion api/tests/opentrons/cli/test_cli.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
"""Test cli execution."""


import json
import tempfile
import textwrap
Expand All @@ -9,8 +11,17 @@

import pytest
from click.testing import CliRunner
from opentrons.util.performance_helpers import _get_robot_context_tracker


from opentrons.cli.analyze import analyze
# 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]

from opentrons.cli.analyze import analyze # noqa: E402


def _list_fixtures(version: int) -> Iterator[Path]:
Expand Down Expand Up @@ -242,3 +253,24 @@ def test_python_error_line_numbers(
assert result.json_output is not None
[error] = result.json_output["errors"]
assert error["detail"] == expected_detail


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")

before_analysis = len(context_tracker._storage) # type: ignore[attr-defined]

_get_analysis_result([protocol_source_file])

assert len(context_tracker._storage) == before_analysis + 1 # type: ignore[attr-defined]
28 changes: 28 additions & 0 deletions api/tests/opentrons/util/test_performance_helpers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
"""Tests for performance_helpers."""

from pathlib import Path
from opentrons_shared_data.performance.dev_types import RobotContextState
from opentrons.util.performance_helpers import (
StubbedTracker,
_get_robot_context_tracker,
)


def test_return_function_unchanged() -> None:
"""Test that the function is returned unchanged when using StubbedTracker."""
tracker = StubbedTracker(Path("/path/to/storage"), True)

def func_to_track() -> None:
pass

assert (
tracker.track(RobotContextState.ANALYZING_PROTOCOL)(func_to_track)
is func_to_track
)


def test_singleton_tracker() -> None:
"""Test that the tracker is a singleton."""
tracker = _get_robot_context_tracker()
tracker2 = _get_robot_context_tracker()
assert tracker is tracker2
4 changes: 4 additions & 0 deletions performance-metrics/src/performance_metrics/__init__.py
Original file line number Diff line number Diff line change
@@ -1 +1,5 @@
"""Opentrons performance metrics library."""

from .robot_context_tracker import RobotContextTracker

__all__ = ["RobotContextTracker"]
34 changes: 1 addition & 33 deletions performance-metrics/src/performance_metrics/datashapes.py
Original file line number Diff line number Diff line change
@@ -1,40 +1,8 @@
"""Defines data classes and enums used in the performance metrics module."""

from enum import Enum
import dataclasses
from typing import Tuple


class RobotContextState(Enum):
"""Enum representing different states of a robot's operation context."""

STARTING_UP = 0, "STARTING_UP"
CALIBRATING = 1, "CALIBRATING"
ANALYZING_PROTOCOL = 2, "ANALYZING_PROTOCOL"
RUNNING_PROTOCOL = 3, "RUNNING_PROTOCOL"
SHUTTING_DOWN = 4, "SHUTTING_DOWN"

def __init__(self, state_id: int, state_name: str) -> None:
self.state_id = state_id
self.state_name = state_name

@classmethod
def from_id(cls, state_id: int) -> "RobotContextState":
"""Returns the enum member matching the given state ID.

Args:
state_id: The ID of the state to retrieve.

Returns:
RobotContextStates: The enum member corresponding to the given ID.

Raises:
ValueError: If no matching state is found.
"""
for state in RobotContextState:
if state.state_id == state_id:
return state
raise ValueError(f"Invalid state id: {state_id}")
from opentrons_shared_data.performance.dev_types import RobotContextState


@dataclasses.dataclass(frozen=True)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,13 @@

from typing_extensions import ParamSpec
from collections import deque
from performance_metrics.datashapes import RawContextData, RobotContextState
from performance_metrics.datashapes import (
RawContextData,
)
from opentrons_shared_data.performance.dev_types import (
RobotContextState,
SupportsTracking,
)

P = ParamSpec("P")
R = TypeVar("R")
Expand All @@ -38,13 +44,15 @@ def _get_timing_function() -> Callable[[], int]:
timing_function = _get_timing_function()


class RobotContextTracker:
class RobotContextTracker(SupportsTracking):
"""Tracks and stores robot context and execution duration for different operations."""

def __init__(self, storage_file_path: Path, should_track: bool = False) -> None:
FILE_NAME = "context_data.csv"

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_file_path
self._storage_file_path = storage_location / self.FILE_NAME
self._should_track = should_track

def track(self, state: RobotContextState) -> Callable: # type: ignore
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from pathlib import Path
import pytest
from performance_metrics.robot_context_tracker import RobotContextTracker
from performance_metrics.datashapes import RobotContextState
from opentrons_shared_data.performance.dev_types import RobotContextState
from time import sleep, time_ns
from unittest.mock import patch

Expand All @@ -19,7 +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_file_path=tmp_path, should_track=True)
return RobotContextTracker(storage_location=tmp_path, should_track=True)


def test_robot_context_tracker(robot_context_tracker: RobotContextTracker) -> None:
Expand Down Expand Up @@ -242,8 +242,7 @@ def operation_without_tracking() -> None:

async def test_storing_to_file(tmp_path: Path) -> None:
"""Tests storing the tracked data to a file."""
file_path = tmp_path / "test_file.csv"
robot_context_tracker = RobotContextTracker(file_path, should_track=True)
robot_context_tracker = RobotContextTracker(tmp_path, should_track=True)

@robot_context_tracker.track(state=RobotContextState.STARTING_UP)
def starting_robot() -> None:
Expand All @@ -263,7 +262,7 @@ def analyzing_protocol() -> None:

robot_context_tracker.store()

with open(file_path, "r") as file:
with open(robot_context_tracker._storage_file_path, "r") as file:
lines = file.readlines()
assert (
len(lines) == 4
Expand Down
1 change: 1 addition & 0 deletions robot-server/Pipfile
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ sqlalchemy2-stubs = "==0.0.2a21"
# limited by tavern
python-box = "==6.1.0"
types-paho-mqtt = "==1.6.0.20240106"
performance-metrics = {file = "../performance-metrics", editable = true}

[packages]
anyio = "==3.7.1"
Expand Down
Loading
Loading