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.

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): publish pause and delay commands in python and JSON #3310

Merged
merged 4 commits into from
Apr 11, 2019

Conversation

b-cooper
Copy link
Contributor

@b-cooper b-cooper commented Apr 5, 2019

This adds an optional message parameter to the delay function in the python apiv2. The message is
published to the run log appended to the standard timing message eg "Delay for 10m 2s".

In addition, the JSON executor for apiv1 and apiv2 is now capable of receiving messages for both delays and pauses. This will allow PD user's input within Pause step messages to be presented in the App's run log regardless of the duration (including indefinite holds) of a pause.

Closes #3308

##Changelog

  • new optional msg param for python APIv2 delay function
  • apiV1 will now call robot.comment with the message associated with a timed "delay" command in a JSON protocol, AND it will now present the time in the run log, regardless of the presence of an optional message
  • apiV2 will now pass the optional message, from a timed "delay" command in JSON, to the new context.delay optional message parameter which will log in the RA.

Review Requests

  • do python protocol pauses still behave appropriately (only change was to apiV2)
  • do JSON protocols present messages for pauses in APIv1
  • do JSON protocols present messages for pauses in APIv2

This adds an optional message parameter to the delay function in the python apiv2. The message is
published to the run log appended to the standard timing message. The json executor for apiv1 and
apiv2 is capable of receiving messages for both delays and pauses. This will allow PD user's input
Pause step messages to be presented in the App's run log regardless of the duration (including
indefinite holds) of a pause.

Closes #3308
@b-cooper b-cooper added feature Ticket is a feature request / PR introduces a feature api Affects the `api` project ready for review labels Apr 5, 2019
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Code LGTM

@codecov
Copy link

codecov bot commented Apr 5, 2019

Codecov Report

Merging #3310 into edge will decrease coverage by 0.62%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #3310      +/-   ##
==========================================
- Coverage   53.63%   53.01%   -0.63%     
==========================================
  Files         719      737      +18     
  Lines       21017    22088    +1071     
==========================================
+ Hits        11272    11709     +437     
- Misses       9745    10379     +634
Impacted Files Coverage Δ
opentrons/commands/commands.py 94.68% <0%> (-0.49%) ⬇️
app/src/components/ModuleItem/index.js 0% <0%> (ø) ⬆️
...mponents/InstrumentSettings/ModulesCardContents.js 0% <0%> (ø) ⬆️
app/src/components/InstrumentSettings/index.js 0% <0%> (ø) ⬆️
...ponents/InstrumentSettings/AttachedPipettesCard.js 0% <0%> (ø) ⬆️
...mponents/InstrumentSettings/AttachedModulesCard.js 0% <0%> (ø) ⬆️
update-server/otupdate/balena/bootstrap.py 0% <0%> (ø)
update-server/otupdate/buildroot/__main__.py 0% <0%> (ø)
update-server/otupdate/balena/__init__.py 0% <0%> (ø)
update-server/otupdate/balena/install.py 0% <0%> (ø)
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f37cba...dd807da. Read the comment docs.

Copy link
Contributor

@Laura-Danielle Laura-Danielle left a comment

Choose a reason for hiding this comment

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

Tested on 🌆 . @IanLondon said to wait to merge this until his PR is in

@IanLondon
Copy link
Contributor

Please don't merge until #3312 thanks!

Copy link
Contributor

@IanLondon IanLondon left a comment

Choose a reason for hiding this comment

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

api v1 timed delays make a message string but doesn't use it as a message

robot.pause(msg=message)
else:
text = f'Delaying for {datetime.timedelta(seconds=wait)}'
if message:
text = f"{text}. {message}"
Copy link
Contributor

Choose a reason for hiding this comment

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

this text var doesn't go anywhere, so APIv1 runs don't give a message for timed delays

@@ -169,14 +169,15 @@ def dispatch_json(context: ProtocolContext, # noqa(C901)
pipette_name = protocol_pipette_data.get('model')

if command_type == 'delay':
wait = params['wait']
wait = params.get('wait')
Copy link
Contributor

Choose a reason for hiding this comment

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

wait is required in schema v1/2/3, so we can do params['wait'] no need to get. Message is optional so get is right there

Copy link
Contributor

@IanLondon IanLondon left a comment

Choose a reason for hiding this comment

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

⏲️

@b-cooper b-cooper merged commit 5656d65 into edge Apr 11, 2019
@mcous mcous deleted the api_json-pause-feature-parity branch July 16, 2019 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the `api` project feature Ticket is a feature request / PR introduces a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants