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

fix(api): add supported wavelengths to runtime error when initializing the plate reader. #16797

Merged

Conversation

vegano1
Copy link
Contributor

@vegano1 vegano1 commented Nov 13, 2024

Overview

Add the supported wavelengths to the runtime error raised when an invalid wavelength is provided.

Closes: RQA-3568

Test Plan and Hands on Testing

  • Make sure the supported wavelengths are reported in the runtime error

Changelog

  • add InvalidWavelengthError to denote incompatible wavelengths with the current plate reader.
  • use the InvalidWavelengthError and format a message to clarify what wavelengths can be used.

Review requests

  • I'm open to word smith's suggestions

Risk assessment

low, log only change

@vegano1 vegano1 requested a review from a team as a code owner November 13, 2024 20:25
@vegano1 vegano1 requested a review from ecormany November 13, 2024 20:25
Copy link
Contributor

@ecormany ecormany left a comment

Choose a reason for hiding this comment

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

Nice, so simple! Would love to be able to surface this in analysis, but that's for another day…

…nitialize.py

Co-authored-by: Ed Cormany <edward.cormany@opentrons.com>
Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

All my homies love actionability.

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.

Comments inline, but should use a protocol engine error and maybe massage the formatting of the wavelengths.

@@ -70,7 +70,10 @@ async def execute(self, params: InitializeParams) -> SuccessData[InitializeResul
supported_wavelengths
)
if unsupported_wavelengths:
raise ValueError(f"Unsupported wavelengths: {unsupported_wavelengths}")
raise ValueError(
Copy link
Member

Choose a reason for hiding this comment

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

This should be an error from protocol_engine.errors that gets associated with an error code - maybe invalid protocol data, but ok to fall back to 4000 general error instead. If it's just a raw exception it will get a scarier UI.

Also this seems like it's just dumping a set to string which will look like this: "Unsupported wavelengths: {1000, 1400, 1200}. Use one of {500, 800, 1000} instead." Feels a little terse, maybe make it {', '.join([str(wavelength) + 'nm' for wavelength in unsupported_wavelengths]) instead?

@vegano1
Copy link
Contributor Author

vegano1 commented Nov 14, 2024

@sfoster1 @ecormany

Added InvalidWavelengthError and fixed up the formatting, here's what that looks like.

Screen Shot 2024-11-14 at 9 13 13 AM

@vegano1 vegano1 requested a review from sfoster1 November 14, 2024 14:29
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.

Excellent, thank you!

@vegano1 vegano1 merged commit 0ae0414 into chore_release-8.2.0 Nov 14, 2024
21 checks passed
@vegano1 vegano1 deleted the add-supported-wavelengths-to-runtime-error branch November 14, 2024 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants