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(app,api): allow rich version specification for python protocols #4358

Merged
merged 3 commits into from
Nov 6, 2019

Conversation

sfoster1
Copy link
Member

This allows and requires apiv2 users to have

'apiLevel': '2.0'

in their metadata dict.

This is parsed and stored along with the protocol and passed along with RPC.
This allows specification of API minor versions.

Closes #4338

This allows and requires apiv2 users to have

'apiLevel': '2.0'

in their metadata dict.

This is parsed and stored along with the protocol and passed along with RPC.
This allows specification of API minor versions.

Closes #4338
@sfoster1 sfoster1 added app Affects the `app` project api Affects the `api` project refactor labels Oct 31, 2019
@sfoster1 sfoster1 requested review from a team October 31, 2019 21:09
@codecov
Copy link

codecov bot commented Nov 1, 2019

Codecov Report

Merging #4358 into edge will decrease coverage by 0.4%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #4358      +/-   ##
==========================================
- Coverage   56.05%   55.65%   -0.41%     
==========================================
  Files         896      897       +1     
  Lines       25125    25410     +285     
==========================================
+ Hits        14085    14142      +57     
- Misses      11040    11268     +228
Impacted Files Coverage Δ
app/src/robot/reducer/session.js 100% <ø> (ø) ⬆️
app/src/robot/actions.js 90.66% <ø> (ø) ⬆️
...omponents/CalibrateLabware/ConfirmModalContents.js 0% <0%> (ø) ⬆️
app/src/robot/api-client/client.js 84.5% <60%> (-0.67%) ⬇️
components/src/instrument/PipetteSelect.js 9.61% <0%> (-3.21%) ⬇️
...steplist/formLevel/stepFormToArgs/mixFormToArgs.js 2.85% <0%> (-1.91%) ⬇️
opentrons/api/session.py 78.76% <0%> (-0.71%) ⬇️
opentrons/protocols/parse.py 83.6% <0%> (-0.36%) ⬇️
protocol-designer/src/components/FilePage.js 0% <0%> (ø) ⬆️
opentrons/protocols/types.py 100% <0%> (ø) ⬆️
... and 12 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 04d8fea...bf8d875. 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.

Right now in simulation I get an error when I run an api v1 protocol with a version level 2.0, I think we should modify get_version to return api versions based on the feature flag. More detail in-line

@@ -46,7 +50,7 @@ def _parse_python(
filename=ast_filename)
metadata = extract_metadata(parsed)
protocol = compile(parsed, filename=ast_filename, mode='exec')
version = infer_version(metadata, parsed)
version = get_version(metadata, parsed)
Copy link
Contributor

Choose a reason for hiding this comment

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

For this function, I think we should only evaluate the metadata blob for a version if we are in server api v2. For server api v1 we should default to infer the version using AST --> this will allow simulation to still be able to differentiate protocols while we finish up backcompat.

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.

Talked F2F with Seth and realized that the v1/v2 feature flag won't be relevant in our next prod release so my comment wouldn't matter at that point.

Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

JS side LGTM

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.

looks good, tested on 🌆 2.0

update.apiLevel = apiSession.api_level || 1
if (Array.isArray(apiSession.api_level)) {
update.apiLevel = apiSession.api_level
} else if (apiSession.api_level) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it's obvious to ppl but might be good to have comment explaining that this case is to support back-compat

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@sfoster1 sfoster1 merged commit b0adef5 into edge Nov 6, 2019
@sfoster1 sfoster1 deleted the api_allow-rich-versioning branch November 7, 2019 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the `api` project app Affects the `app` project refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

APIv2 Versioning: apiLevel should be parsed for major.minor and should be required
4 participants