refactor(api, robot-server): move feature flag file access outside of hardware_control #13963
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…trol instantiators are responsible for passing them in
…are-code # Conflicts: # api/src/opentrons/hardware_control/ot3api.py
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## edge #13963 +/- ##
==========================================
- Coverage 70.59% 70.49% -0.11%
==========================================
Files 2506 2507 +1
Lines 71081 71083 +2
Branches 8907 8916 +9
==========================================
- Hits 50179 50108 -71
- Misses 18723 18790 +67
- Partials 2179 2185 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
sfoster1
requested changes
Nov 14, 2023
| @@ -111,6 +112,7 @@ def __init__( | |||
| backend: Union[Controller, Simulator], | |||
| loop: asyncio.AbstractEventLoop, | |||
| config: RobotConfig, | |||
| feature_flags: HardwareFeatureFlags = HardwareFeatureFlags(), | |||
There was a problem hiding this comment.
this probably wants to be Optional[HardwareFeatureFlags] = None because this code will create one default HardwareFeatureFlags() at function parse time that will be reused lol
| @@ -170,6 +173,7 @@ async def build_hardware_controller( | |||
| port: Optional[str] = None, | |||
| loop: Optional[asyncio.AbstractEventLoop] = None, | |||
| firmware: Optional[Tuple[pathlib.Path, str]] = None, | |||
| feature_flags: HardwareFeatureFlags = HardwareFeatureFlags(), | |||
| @@ -249,6 +258,7 @@ async def build_hardware_simulator( | |||
| config: Optional[Union[RobotConfig, OT3Config]] = None, | |||
| loop: Optional[asyncio.AbstractEventLoop] = None, | |||
| strict_attached_instruments: bool = True, | |||
| feature_flags: HardwareFeatureFlags = HardwareFeatureFlags(), | |||
| config: OT3Config, | ||
| use_usb_bus: bool = False, | ||
| check_updates: bool = True, | ||
| feature_flags: HardwareFeatureFlags = HardwareFeatureFlags(), |
| @@ -274,6 +283,7 @@ def __init__( | |||
| usb_driver: Optional[SerialUsbDriver] = None, | |||
| eeprom_driver: Optional[EEPROMDriver] = None, | |||
| check_updates: bool = True, | |||
| feature_flags: HardwareFeatureFlags = HardwareFeatureFlags(), | |||
| @hardware_feature_flags.setter | ||
| def hardware_feature_flags(self, feature_flags: HardwareFeatureFlags) -> None: | ||
| self._feature_flags = feature_flags | ||
| if isinstance(self._backend, OT3Controller): |
There was a problem hiding this comment.
let's just do it for the simulator too
sfoster1
approved these changes
Nov 15, 2023
fsinapi
deleted the
RET-1382-Refactor-feature-flags-out-of-hardware-code
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
As a part of the long-term goal of the hardware controller process split, we need to address simultaneous access & modification of the feature flags file on a robot. This PR takes out any parts of the
hardware_controlsubmodule in theopentronspackage that directly access the feature flags file, replacing them with a dataclass that just gets populated by the caller of the factory functions. This gets used to dictate whether the pipette handlers use the new or old aspiration functions, as well as a few Flex-specific hardware functions.Test Plan
To verify that the settings are being updated in the API correctly, I pushed on a robot, used HTTP to change the advanced setting requiring the estop, and disconnected the estop to ensure that no error popped up.
CI tests should cover that the settings are still being set/read the right way.
Ran the REPL on a robot and changed some settings in
feature_flags.jsonto ensure they are reflected in the constructed API.Changelog
Review requests
I don't like this interface but I don't see an elegant alternative. Injecting into the robot configs seems inappropriate imo. Any strong opinion on an alternative approach is appreciated.
Risk assessment
Medium, mostly if I missed any hardware API initializers somewhere in the repo.