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.

Sign up for GitHub

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

Jump to bottom

docs(api): CSV runtime parameters #15910

Merged
merged 12 commits into from
Aug 20, 2024
Merged

docs(api): CSV runtime parameters #15910

merged 12 commits into from
Aug 20, 2024

Conversation

ecormany
Copy link
Contributor

@ecormany ecormany commented Aug 7, 2024 •
edited
Loading

Overview

Documentation for defining and using CSV runtime parameters.

Test Plan and Hands on Testing

  • Sandbox
  • Use case code passes simulation

Changelog

  • Adds section to Defining Parameters
  • Adds section to Using Parameters
  • New page: Use Case – Cherrypicking
  • Adds CSVParameter class to API Reference, with newly edited docstrings

Review requests

  • Lots of new words. Words good?
  • A handful of code snippets outside of the use case. Check for validity and Pythonicity.

Risk assessment

nil, docs

@ecormany ecormany added docs papi-v2 Python API V2 labels Aug 7, 2024
@ecormany ecormany marked this pull request as ready for review August 16, 2024 02:12
@ecormany ecormany requested a review from a team as a code owner August 16, 2024 02:12
jbleon95
jbleon95 approved these changes Aug 16, 2024
View reviewed changes
Copy link
Contributor

@jbleon95 jbleon95 left a comment

Choose a reason for hiding this comment

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

All the CSV code related samples and doc strings look good, a couple of minor nitpicks/notes but otherwise 👍

api/docs/v2/parameters/defining.rst Outdated Show resolved Hide resolved
api/docs/v2/parameters/use_case_cherrypicking.rst Show resolved Hide resolved
api/src/opentrons/protocols/parameters/csv_parameter_interface.py Outdated Show resolved Hide resolved
jwwojak
jwwojak reviewed Aug 16, 2024
View reviewed changes
Copy link
Contributor

@jwwojak jwwojak left a comment

Choose a reason for hiding this comment

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

Thanks for the opportunity to review. I have to cut this short because of an upcoming appointment. Left some suggestions for consideration. Glad to look at it more later as needed.

api/docs/v2/parameters/defining.rst Outdated Show resolved Hide resolved
api/docs/v2/parameters/defining.rst Show resolved Hide resolved
api/docs/v2/parameters/use_case_cherrypicking.rst Show resolved Hide resolved
api/docs/v2/parameters/use_case_cherrypicking.rst Outdated Show resolved Hide resolved
api/docs/v2/parameters/use_case_cherrypicking.rst Outdated Show resolved Hide resolved
api/docs/v2/parameters/use_case_cherrypicking.rst Show resolved Hide resolved
jwwojak
jwwojak approved these changes Aug 19, 2024
View reviewed changes
Copy link
Contributor

@jwwojak jwwojak left a comment

Choose a reason for hiding this comment

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

LGTM with 2 simple comments for consideration.

:shipit:

api/docs/v2/parameters/using_values.rst

Remember that CSV parameters don't have default values. Accessing CSV data in any of the above ways will prevent protocol analysis from completing until you select a CSV file and confirm all runtime parameter values during run setup.

You can use a try–except block to work around this and provide the data needed for protocol analysis. First, add ``from opentrons.protocol_api import RuntimeParameterRequiredError`` at the top of your protocol. Then catch the error like this::
Copy link
Contributor

Choose a reason for hiding this comment

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

Should "try-except" have code formatting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it could, but it wouldn't include the dash, so it would look like try–except which i think is kinda weird.

api/docs/v2/parameters/using_values.rst Outdated Show resolved Hide resolved
sanni-t
sanni-t approved these changes Aug 19, 2024
View reviewed changes
Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

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

Just the comment about the link to file object page.
Rest all looks great!

api/docs/v2/parameters/using_values.rst Outdated Show resolved Hide resolved
@ecormany ecormany merged commit ef3769d into chore_release-8.0.0 Aug 20, 2024
20 of 21 checks passed
@ecormany ecormany deleted the docs-csv-rtp branch August 20, 2024 19:18
@alexjoel42
Copy link
Contributor

@ecormany , https://github.com/Opentrons/opentrons/blame/502bbc6d044ecc8821767d2330977ed8717eebfc/api/docs/v2/parameters/use_case_cherrypicking.rst#L122-L128
There's a parentheses on line 128 that should be on 125.

@ecormany ecormany mentioned this pull request Aug 22, 2024
Merged
ecormany added a commit that referenced this pull request Aug 22, 2024
@ecormany
docs(api): fix syntax error in cherrypicking use case (#16100)
9d98cfc 
# Overview

Thanks to @alexjoel42 for [noticing this syntax
error](#15910 (comment))
in the cherrypicking use case. I think I tried to shift loading the
trash bin lower in the protocol and left it one line short of where it
needed to go.

## Test Plan and Hands on Testing

I simulated the whole code block, no modifications except indentation,
promise.

## Changelog

🆙 

## Risk assessment

none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jwwojak jwwojak jwwojak approved these changes

@jbleon95 jbleon95 jbleon95 approved these changes

@sanni-t sanni-t sanni-t approved these changes

Assignees
No one assigned
Labels
docs papi-v2 Python API V2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ecormany @alexjoel42 @sanni-t @jbleon95 @jwwojak