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.

Sign up for GitHub

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

fix(modules): use parse from packaging module #14732

Merged
merged 8 commits into from
Apr 1, 2024
Merged

fix(modules): use parse from packaging module #14732

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ac8cf4a
return has update for unexpected module fw filename
caila-marashaj Mar 21, 2024
994f07b
change regex instead
caila-marashaj Mar 21, 2024
6f812fa
revert regex and try catch version update
caila-marashaj Mar 22, 2024
40a6a5a
use packaging parse
caila-marashaj Mar 25, 2024
e2886e4
parse version outside of module abc
caila-marashaj Mar 26, 2024
bcf9f53
move check fw version test to api
caila-marashaj Mar 29, 2024
f9bf475
rm unnecessary import
caila-marashaj Apr 1, 2024
a4470d6
restore robot-server code
caila-marashaj Apr 1, 2024
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
19 changes: 13 additions & 6 deletions api/src/opentrons/hardware_control/modules/mod_abc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@
import asyncio
import logging
import re
from pkg_resources import parse_version
from typing import ClassVar, Mapping, Optional, cast, TypeVar

from typing import ClassVar, Mapping, Optional, TypeVar
from packaging.version import InvalidVersion, parse, Version
from opentrons.config import IS_ROBOT, ROBOT_FIRMWARE_DIR
from opentrons.drivers.rpi_drivers.types import USBPort

Expand All @@ -16,6 +15,14 @@
TaskPayload = TypeVar("TaskPayload")


def parse_fw_version(version: str) -> Version:
Copy link
Contributor

@DerekMaggio DerekMaggio Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What is the difference when using parse instead of instantiating a Version object directly?
The answer is nothing, absolutely nothing.
https://github.com/pypa/packaging/blob/main/src/packaging/version.py#L47-L56

It is for backward compatibility when parse used to return a Version or a LegacyVersion object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I was mostly using it here to detect a bad version name so we can default it to 0.0.0

Copy link
Contributor

Choose a reason for hiding this comment

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

No, your function is totally good. What I mean is all packaging.version.parse(string_version) does is call Version(string_version) under the hood. Ignore me lol

try:
device_version = parse(version)
except InvalidVersion:
device_version = parse("v0.0.0")
return device_version


class AbstractModule(abc.ABC):
"""Defines the common methods of a module."""

Expand Down Expand Up @@ -88,9 +95,9 @@ def get_bundled_fw(self) -> Optional[BundledFirmware]:
def has_available_update(self) -> bool:
"""Return whether a newer firmware file is available"""
if self.device_info and self._bundled_fw:
device_version = parse_version(self.device_info["version"])
available_version = parse_version(self._bundled_fw.version)
return cast(bool, available_version > device_version)
device_version = parse_fw_version(self.device_info["version"])
available_version = parse_fw_version(self._bundled_fw.version)
return available_version > device_version
return False

async def wait_for_is_running(self) -> None:
Expand Down
19 changes: 19 additions & 0 deletions api/tests/opentrons/hardware_control/test_modules.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

from pathlib import Path
from unittest import mock
from packaging.version import Version

from opentrons.hardware_control import ExecutionManager
from opentrons.hardware_control.modules import ModuleAtPort
Expand All @@ -22,6 +23,7 @@
HeaterShaker,
AbstractModule,
)
from opentrons.hardware_control.modules.mod_abc import parse_fw_version
from opentrons.drivers.rpi_drivers.types import USBPort


Expand Down Expand Up @@ -422,3 +424,20 @@ def test_magnetic_module_revision_parsing(revision, model):
)
def test_temperature_module_revision_parsing(revision, model):
assert TempDeck._model_from_revision(revision) == model


@pytest.mark.parametrize(
argnames=["device_version", "expected_result"],
argvalues=[
["v1.0.4", Version("v1.0.4")],
["v0.5.6", Version("v0.5.6")],
["v1.0.4-dhfs", Version("v0.0.0")],
["v3.0.dshjfd", Version("v0.0.0")],
],
)
async def test_catch_invalid_fw_version(
device_version: str,
expected_result: bool,
) -> None:
"""Assert that invalid firmware versions prompt a valid Version object of v0.0.0."""
assert parse_fw_version(device_version) == expected_result
9 changes: 6 additions & 3 deletions robot-server/tests/modules/test_router.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,12 @@
from opentrons.calibration_storage.types import SourceType
from opentrons.hardware_control import HardwareControlAPI
from opentrons.drivers.rpi_drivers.types import USBPort as HardwareUSBPort, PortGroup
from opentrons.hardware_control.modules import MagDeck, ModuleType, MagneticStatus
from opentrons.hardware_control.modules import module_calibration
from opentrons.hardware_control.modules import (
MagDeck,
ModuleType,
MagneticStatus,
module_calibration,
)

from opentrons.types import Point
from opentrons.protocol_engine import ModuleModel
Expand All @@ -25,7 +29,6 @@
UsbPort,
)


_HTTP_API_VERSION: Final = 3


Expand Down
Loading