-
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
feat(api): Add robot context API skeleton #15745
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #15745 +/- ##
=======================================
Coverage 63.70% 63.70%
=======================================
Files 300 300
Lines 15649 15649
=======================================
Hits 9969 9969
Misses 5680 5680
Flags with carried forward coverage won't be shown. Click here to find out more. |
A PR has been opened to address analyses snapshot changes. Please review the changes here: https://github.com/Opentrons/opentrons/pull/ |
There is a class called RobotContextTracker that is part of performance-metrics. If this feature makes more sense to call RobotContext than performance-metrics, the class in performance-metrics should get changed. I was instantly confused that you were using the performance-metrics class. Until I realized you werent. If performance-metrics should be changed just let me know and I can handle it |
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 the signature of move_to
is wrong and also we might want to pull out the reiteration of the function name in the NotImplementedError
s since we'll just have to keep them up to date until merge.
Hey @DerekMaggio yes I think we definitely want to call this the |
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.
Thanks for giving me a preview of this! We do need to make sure that docs builds succeed before merging to edge
— see inline comment. All my other comments are wordsmithing that can be addressed now or later.
|
||
def move_to( | ||
self, | ||
abs_axis_map: Dict[hw_types.Axis, hw_types.AxisMapValue], |
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 stability guarantees are we making about this robot context stuff? Is it the same as the "normal" Python Protocol API, where we preserve backwards compatibility and it's governed by apiLevel
?
If so, I think we should not use anything from hw_types
, or anything defined outside opentrons.protocol_api
, in our public signatures. I can elaborate on this if needed, but going forward, I think we need to draw a really bright line between what is publicly importable and usable, and what is not.
But that doesn't apply if you're intentionally leaving these new APIs loose, raw, and non-guaranteed.
What do you think?
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 has the same stability guarantees as the rest of the API, so you are correct, and we should not use anything defined outside opentrons.protocol_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.
Agree with you both. Will address the better types in the implementation PRs.
# Overview Closes https://opentrons.atlassian.net/browse/EXEC-623 Renames RobotContextTracker and RobotContextState to RobotActivityTracker and RobotActivityState. This is to prevent confusion with the `RobotContext` class added in #15745 # Test Plan - [x] Make sure linting and testing passes
4c29e21
to
bd2f706
Compare
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 looks good to me at this point!
Overview
Initial PR to create a skeleton API which exposes certain methods of a versioned HW api for finer robot control.
Test Plan
ctx._hw_manager
still works as expectedChangelog
Review requests
Check that you're OK with how we access this API using python.
Risk assessment
Should be low-medium as we just changed how the underlying hardware is working as expected.