-
Notifications
You must be signed in to change notification settings - Fork 178
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
API work to enable calibration experience #352
Conversation
…accumulate command messages
… and instruments to limit scope in client comms"
…ment, fix root object instantiation in server/main' '
Codecov Report
@@ Coverage Diff @@
## app-3-0-alpha #352 +/- ##
=================================================
- Coverage 82.98% 82.51% -0.47%
=================================================
Files 73 76 +3
Lines 4737 4851 +114
=================================================
+ Hits 3931 4003 +72
- Misses 806 848 +42
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just bikeshedding and organizational feedback. I think I'd be happiest with MainRouter
living in opentrons.api
that wraps SessionManager, CalibrationMangager, and whatever else
api/opentrons/commands/commands.py
Outdated
@@ -25,14 +25,16 @@ def home(axis): | |||
) | |||
|
|||
|
|||
def aspirate(volume, location, rate): | |||
def aspirate(volume, location, rate, self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename this self
parameter to instrument
(as it is used everywhere)? Also, does it make more sense to have instrument be the first parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^Agreed, using self outside of the context of a class is confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked to @mcous IRL. The signature is this because of the decorator. Possibly we could have implemented actual commands as functions with 'instrument' as an argument, but since they are not, and since the decorator is magic we'll keep magic inside of it and map 'self' to instrument. That will change command builder's signature from 'self' to 'instrument'.
api/opentrons/server/main.py
Outdated
@@ -4,9 +4,10 @@ | |||
import sys | |||
import logging | |||
from opentrons.server.rpc import Server | |||
from opentrons.session import SessionManager | |||
from opentrons.session import Shell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't love the name Shell
... maybe MainRouter
?
Also, re: outgrowing the scope of Session, I like the opentrons.api
proposal the most. Alternatively, opentrons.app-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make it 'opentrons.api'
api/opentrons/session/calibration.py
Outdated
@@ -0,0 +1,25 @@ | |||
PIPETTE_CHANGE_POSITION = (50, 50, 50) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will clean up.
api/opentrons/session/session.py
Outdated
@@ -21,7 +22,7 @@ def __init__(self, loop=None): | |||
SESSION_TOPIC, self._notifications.on_notify) | |||
self.session = None | |||
self.robot = Robot() | |||
# TODO (artyom, 09182017): This is to support the future | |||
# TODO (artyom, 20170918): This is to support the future | |||
# concept of archived sessions. To be reworked when more details | |||
# are available | |||
self.sessions = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we could comment this out to avoid pushing this down the wire?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
@@ -4,6 +4,39 @@ | |||
from datetime import datetime | |||
from opentrons.broker import publish | |||
from opentrons.session import Session | |||
from opentrons.session.session import _accumulate, _get_labware |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we could avoid testing these private methods directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long we use a more descriptive name than Shell
and don't use self
in this context, I'm good to approve. Don't love all the wrappers needed but I get why they're used.
… for commands and implement logic to handle that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important change needed to make sure notifications keep flowing to the client
api/opentrons/session/wrappers.py
Outdated
self.coordinates = well.coordinates(reference=well.parent) | ||
|
||
|
||
class Instrument: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Information needed for the UI:
- number channels
- tip type (volume)
- left or right axis
Also, what is name?
api/opentrons/api/wrappers.py
Outdated
for container in set(containers) | ||
} | ||
|
||
def move_to(self, obj): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep these as pure data classes or else we run the risk of logic creep a la v2 flask server. Can we remove this move_to
method?
While I'm writing this comment, a few extra things:
- I think a more canonical name for these would be "models"
- I worry about the organizational structure that's emerging here:
- We seem to be organizing a little by type of thing (
routers.py
,wrappers.py
) - The alternative would be concern of thing
- We seem to be organizing a little by type of thing (
- I wasn't expecting
opentrons.session
andopentrons.calibration
to both get lumped intoopentrons.api
- I thought we were just going to put
MainRouter
inopentrons.api
- I don't necessarily oppose this change, but I also feel like it wasn't discussed
- My fear here is the same logic creep concerns as enumerated above
- I thought we were just going to put
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move_to
removed. Someone needs to dispatch a call to actual instrument's move_to
. LMK your thought on what the alternative would be.
- Great point. Fixed.
- Models and Routers seems like structural elements of the communication. If I understand you correctly, that would mean
MainRouter
would go tomodels
. Or init maybe? This leaves us withcalibration
andsession
. I would imaginesession
could becomerun
. Happy to have it either way. - When writing
Code contained in opentrons.session has outgrown the scope of the name. I suggest the following options: rename it to opentrons.api (vs. potentially opentrons.protocols in the future, for protocol-related matters)
I meantopentrons.session
sub-package, as inopentrons/session
.
We can keep both, if there is a strong reason to do so.
command_args = dict( | ||
zip( | ||
reversed(inspect.getargspec(command).args), | ||
reversed(inspect.getargspec(command).defaults or []))) | ||
|
||
# TODO (artyom, 20170927): we are doing this to be able to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered a decorator interface more like this?
@commands.publish.both(command=commands.aspirate, map_args={'self': 'instrument'})
def aspirate(self, volume=None, location=None, rate=1.0):
# ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, once we have more than one mapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is one mapping not enough to implement it?
api/opentrons/instruments/pipette.py
Outdated
@@ -813,7 +813,7 @@ def drop_tip(self, location=None, home_after=True): | |||
location = location.bottom(self._drop_tip_offset) | |||
|
|||
@commands.publish.both(command=commands.drop_tip) | |||
def _drop_tip(location): | |||
def _drop_tip(location, self=self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this not match the signature for _pick_up_tip
above?
@commands.publish.both(command=commands.pick_up_tip)
def _pick_up_tip(location, instrument=self):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
api/opentrons/api/routers.py
Outdated
class MainRouter: | ||
def __init__(self, loop=None): | ||
self.session_manager = SessionManager(loop=loop) | ||
self.calibration_manager = CalibrationManager(loop=loop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Main router is going to need a notifications object per the RPC server monitoring:
# rpc.py
async def monitor_events(self, instance):
async for event in instance.notifications:
# ...
I propose that the MainRouter
listens to the notifications of its managers and routes them to the server via its own Notifications
:
- on
SessionManager
notification enqueue{'name': 'session', 'payload': session_instance}
CalibrationManager
doesn't have notifications yet, but if it did:{'name': 'calibration', 'payload': ???}
- I don't know if we'll have an analog to
Session
inCalibrationManager
but we also don't need to worry about it yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely. Thanks for catching this. Will fix and add test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gonna listen to the broker directly and add topic
to the message
…p notification handling in sessionmanager and session, update tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you've got a leftover debugging print call, but I'm 👍 whether or not you remove it
Edit: looks like you caught it before I posted my review
api/opentrons/api/routers.py
Outdated
self.session_manager = SessionManager(loop=loop) | ||
self.calibration_manager = CalibrationManager(loop=loop) | ||
self._unsubscribe = subscribe( | ||
Session.TOPIC, | ||
self._notifications.on_notify) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a change request, but just an idea for the future: how do we feel about polymorphic function arguments? Might be nice to just be able to do subscribe(topic, notificationsInstanceOrHandlerFunction)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had it here: https://github.com/OpenTrons/opentrons/blob/3965637a6b9fcc002428094bcb1507cfb3cfc671/api/opentrons/broker/broker.py#L49 and considered it being too implicit on last code review. Happy to revisit later. cc: @jaredgreene1
|
||
|
||
class SessionManager(object): | ||
def __init__(self, loop=None): | ||
self._notifications = Notifications(loop=loop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
api/opentrons/broker/broker.py
Outdated
@@ -23,6 +23,7 @@ def snooze(self): | |||
|
|||
def on_notify(self, message): | |||
def thread_has_event_loop(): | |||
print(message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a debug line to remove?
publish(Session.TOPIC, self._snapshot()) | ||
|
||
|
||
def _accumulate(iterable): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is doing a union on an iterable of tuples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will expect an iterable of tuple of lists and accumulate lists in each component of a tuple. Sounds like a union, except for this is not a set.
* Add react-router (with redux bindings) to UI * Reorganize container structure with routes - Removes Root container in favor of more specific containers - Adds ConnectedRunControls container for run controls - Adds some basic routing to App.js (which is now top-level comp) * implement run button in run button header, style updates * Scaffold out routes for pipette and deck setup * clean up run controls layout * cleanup sidebar layout, fix run log scroll * minor style fiddling * fix linting errors * Dispatch a successful connect response if already connected * initial pipetteConfig component and styles * static single pipette config component * API work to enable calibration experience (#352) * Implement `get_contaieners` and `get_instruments` and underlying code to accumulate and interpret command messages. * Introduce `MainRouter` as a root client API object and make it contain `calibration_manager` and `session_manager`. * Remove notification handling from `SessionManager` and `Session` and move it up to `MainRouter`, clean up tests. * Add model data classes for labware and instruments to limit scope of client API surface. * Add `location` to commands where appropriate and change `publish` to translate `self` to `location` if needed. * Consolidate Client API related code under `opentrons.api`. * wire up mock props pipette config container * fix linting errors * render pipettes toggle active pipette route in pipette config * added tip probe messages as bottom panel * shrink sidebar slightly, default tip probe state * Fix API make install task and update client remote for MainRouter * Fix resume and reconnect (#346) * start task instead of await in websocket message handler loop to allow concurrent calls; add test for concurrent calls * add test stub for concurrent connections * fix crashes on disconnect, add tests for concurrent connections and disconnect * send messages to different clients independently, test for simultaneous connects and disconnects * refactor how send queues and tasks are being managed, improve tests * Add run timer cleanup to client disconnect * change levels for logging messages, change logging level in main.py handler, remove task from the queue once done * make robot.smoothie_driver private * move init of seen to top, to have it initiaized before we try to create remote objects in parallel for array * Resolve array remotes serially to avoid type resolution race * Calibration API Implementation (#366) * add calibration API tests, make _simulate call exec instead of relying on run with no arguments * add test for serializing class object attributes * Harmonize shape of notifications, reduce state tree * Make notification payload to be a copy of self across the board * Remove Wells from Container data object to reduce Session state tree * Refactor `placeable.get_container` * Don't treat `WellSeries` as container * Make `placeable.get_trace()` a generator * Add functional test with full protocol to test session model * V3a calibration nav panel (#363) * separate container for SetupPanel * list pipette links with optional axis params, list labware with optional params - only show once pipettes are probed * setup panel layout updates, conditionally render labware links and run link * layout cleanup * conditionally disable non tipracks until tipracks have been confirmed * add proptypes * map -> reduce * fix proptype * always render laware list, disable until intruments are calibrated * (v3 alpha, feature) Calibration API client state (#368) * Add instrument and labware state translation to API client * Add private ID field to RemoteObjects * Add actions for CalibrationManager calls and responses * Wire calibration actions and responses to API client * Track calibration API request state in redux store * V3a ui calibrate labware (#369) * separate container for SetupPanel * list pipette links with optional axis params, list labware with optional params - only show once pipettes are probed * initial deckmap for rework * confirm deck layout, styles * separate deckmap function * initial jod modal nested route, container, component. Remove position, replace with numerical slots, add ids for legacy slot names * convert labware wrapper elements to links, add active container/link styling * jog modal container, component, styling, and placeholder cotent and route * cleanup labware labels * jog modal button placeholder, general cleanup * render default pipette imgs, reset mock state after demo, sync tip prob and labware calibration notifications to state * (v3 alpha) Calibration UI MVP (#372) * static jog buttons for wriring * minor ui style updates, nav icons now router links, splash screen for upload page * render placeholder images for bradford assay labware * remove navlinks from sidebar, labware, pipettes in favor of current instrument and current labware * Wire up instrument calibration and some of labware confirmation * Wire some labware selectors to SetupPanel and DeckConfig * Wire labware review state+button, simplify bool prop names * update setup panel styles, default splash screen and route * nest tip probe notifications within pipette config * fix lint, troubleshoot Prepare for tip probe props * Wire up labware confirmation for all Yes clicks (no jog) * Wire up jog modal and confirmation button / reducers * labware selected, disabled, active, confirmed styles * Fix API calls for calibration and run (and unit tests) * Fix lint errors in Labware.js and .css * Driver, Pose Tracker and Calibration procedure (#374) * driver started * Implements working pipette probing Create a script that serves as a functional version of pipette probing which still needs to be logically placed into correct location in code base. Begin implementation of Gantry and Base classes. * Expand on Probe and Base classes * enable OTtwo to run existing protocols * set the slot and mount offsets * Enable probing with both pipettes Adjust offsets and probe position / dimensions to enable probing for both pipettes. Also adds mounts to pipettes (this is a hack) to enable offset values to be pulled from instruments so they can be used in calibration. This update of the global dicts should also allow calibration to persist between probing and running. * corrects deck offset * driver started * Implements working pipette probing Create a script that serves as a functional version of pipette probing which still needs to be logically placed into correct location in code base. Begin implementation of Gantry and Base classes. * Expand on Probe and Base classes * enable OTtwo to run existing protocols * set the slot and mount offsets * Enable probing with both pipettes Adjust offsets and probe position / dimensions to enable probing for both pipettes. Also adds mounts to pipettes (this is a hack) to enable offset values to be pulled from instruments so they can be used in calibration. This update of the global dicts should also allow calibration to persist between probing and running. * corrects deck offset * driver and api integration work for the demo * added axis to models.Instrument * add jog functionality * change jog function name * remove robot from session, make it home before run * use reset() instead of hard reset in robot test fixture * Use driver.(dis)connect instead of toggling simulating flag directly * Resin for app-3-0 (Chore) (#347) * attempting image build * attempting image build * fixing Dockerfile.template * fixing Dockerfile.template * fixing Dockerfile.template * fixing Dockerfile.template * fixing Dockerfile.template * fixing Dockerfile.template * refactor server start * refactor server start * refactor server start * refactor server start * refactor server start * adding motion testing_script * resin image build * resin image build * resin image build * resin image build * resin README added * slight refactor for PR * docs adjustment * docs adjustment * moved filed * linting fix * Change server port and fix life-testing script * add TODOs to Dockerfile.template * Resin for app-3-0 (Chore) (#347) * atempting image build * attempting image build * fixing Dockerfile.template * fixing Dockerfile.template * fixing Dockerfile.template * fixing Dockerfile.template * fixing Dockerfile.template * fixing Dockerfile.template * refactor server start * refactor server start * refactor server start * refactor server start * refactor server start * adding motion testing_script * resin image build * resin image build * resin image build * resin image build * resin README added * slight refactor for PR * docs adjustment * docs adjustment * moved filed * linting fix * Change server port and fix life-testing script * add TODOs to Dockerfile.template * make ot_data live on USB on pi * flip well positions for database migration * Functional tests for API - Added position tracking and position log for driver when running in simulate mode for testing purposes - Re-wired calibration_manager.jog to call calibration_functions.jog_instrument properly - Added API functional tests in api/test_functional.py consistent with common use cases. - Added bradford assay edited to fit OT-2 deck * rotate containers for ot-alpha and fix well offsets * Make moving more consistent and tidy things up Make instrument movement more consistent by using it's mover instead of the driver. Also remove a few print statements and an unused functionality in the driver. * prevent homing of nonexistent pip * fix tests * modify target values for homing * Add methods for updating z lenth of pipette in pose tracker instead of adding on the fly * use correct variable in direct movement strategy * adjust pipette position with tip offset * fix ui calibration setting and track relative position * Fixes for relative path access in posetracker, add all files starting with . to gitignore * fix logical errors in relative position access * adjust offsets for avagadro and toss probing deltas * adjust offsets in prep for KV demo * fix height of non-moving pipette during move * functioning calibration cli * output cleanup * move pipette by applying transformation from deck branch of a posetracker bypassing instrument branch of pose-tracker, adjust container definitions based on real life tests, add smoke test to api/test_integration, change tip pickup move sequence and travel, change tip offset to be applied to move command instead of being tracked in posetracker * functioning calibration cli * output cleanup * move pipette by applying transformation from deck branch of a posetracker bypassing instrument branch of pose-tracker, adjust container definitions based on real life tests, add smoke test to api/test_integration, change tip pickup move sequence and travel, change tip offset to be applied to move command instead of being tracked in posetracker * refactorings after review with Ben * integrate factory calibration with pose tracker, make changes in pose tracker to account for transformation alongside with translation, fix tests * 1) Improved smoothie driver to not send duplicate commands if we are already at the point for an axis 2) caching position so we don’t have to make serial calls every time we need position, reduces the risk of error in serial comms which are shaky 3) Fix a bug that was introduced after we made `position` a tuple, after which `+` operator did concatenation, not a vector sum 4) Improved `api/test_integration` to aggregate move logs by axis for easier analysis 5) Added test to confirm trashbox is present in session containers when drop_tip is called with no arguments in the protocol (cc @mike @jared) 6) Fixed api tests WRT to connect attempts being made to actual device - * Add back in assert statement for test * making smoke test work, refactoring calibration tests, refactoring tip probe * fixes
Add API classes and methods to support calibration user experience.
Reference: https://docs.google.com/document/d/1zyliqWSrr2l6VIeGMT2nbxgyLDEsjkshxuosVd-XCIk/edit#
changelog
CalibrationManager
class with stubs for future calibration-related API calls. Currently hassession_manager
andcalibration_manager
data attributes.Shell
as a container class forSessionManager
,CalibrationManager
and potentially other classes to address client needs.get_containers
andget_instruments
toSession
to return containers, instruments and their pairwise interactions for a protocol.Instrument
,Container
,Well
wrappers to limit scope of original classes used in Protocols API and expose data attributes and functions needed for UI client.review requests
Instrument
,Container
,Well
wrappers serve two purposes: limit the scope of client-facing API, clean-up the surface of existing classes without introducing too many changes in the code base. Your thoughts on the approach are appreciated.Code contained in
opentrons.session
has outgrown the scope of the name. I suggest the following options:opentrons.api
(vs. potentiallyopentrons.protocols
in the future, for protocol-related matters)opentrons.server