-
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): move-to-slot JSON protocol command #3242
Merged
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
b352662
feat(api): api2: add z margin override to move_to
sfoster1 7e843a3
add tests
IanLondon 177be57
feat(api): support move-to-well command
IanLondon 965f759
move-to-slot not move-to-well
IanLondon 83f7210
APIv1 move-to-slot command working; APIv2 TODO "strategy"
IanLondon 6429d32
wip: use z-margin not strategy
IanLondon 3f054b3
fix arc profile
sfoster1 f05ff24
merge seth's fixes to motion profile
IanLondon ff23726
separate force_direct vs z_safety
IanLondon af4aa05
rename z_margin_override -> minimum_z_height
IanLondon 8cf6d9f
fixup docstrings
IanLondon b1d91ae
revert labware names to lowercase
IanLondon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
File renamed without changes.
Oops, something went wrong.
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.
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't we do
slot in context.deck
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 that's too permissive b/c it will work fine with integers even though the schema says it must be a string:
"enum": ["1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12"]
.But I'm pretty sure
1 in context.deck
and"1" in context.deck
will both be True... didn't test thoughProbably the best thing to do is to throw before execution even begins if the protocol does not conform to a schema supported by the executor, but until we have something like that I don't want users to run out-of-spec JSON and have things work fine - then they'll not bother making it a string and we'll have a tough time making a migration script to move their protocols to future JSON schema versions :/
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.
Yeah I agree with that but still, "what the deck will accept" is exactly the source of truth we should be checking here, not duplicating logic.
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 agree with Seth about duplicating logic and to extend that thought, this would be a great place to start using the ot2Standard.json deck definition in shared data. And a helper method to grab the slot ids from the definition would return an array of strings by default.
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.
The source of truth for the JSON executor about what inputs are valid should always be the JSON Protocol schema def, not the Deck class of a certain version of the Python API, or anything else outside the JSON Protocol schema def matching the protocol version of the uploaded protocol.
Permissivity in the executors is dangerous. Our JSON Protocol schema is not intended to be read only by the JSON executor(s) in our Python server/API. For example, we might want to display commands info in Run App or Protocol Library from the JSON file directly. If we encourage protocols to deviate from the schema definition by having a permissive executor, our JSON protocols "in the wild" will become much harder to work in all these cases. As a superuser, if my protocol runs fine when I use integers for slots, I'm not going to really care if I'm going against the schema -- I likely won't even know that my protocol is invalid because it runs fine on the robot. Then if Run App wants to add a new feature that reads the
slots
param and expects a string, we have to either make users convert their invalid protocols to string slots, or handle that out-of-spec case to the application.I think the long-term right thing to do is adding JSON schema validation checks to the executors, which would throw an error if the uploaded protocol does not validate under the JSON Protocol schema that matches that uploaded protocol's schema version (
"protocol-schema" / "protocolSchema"
key). We should probably do that pretty soon. Then, we can take this type of "is the input valid?" assertions out of the executors.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.
#3250