feat(api): add the touch_tip(mm_from_edge=...) argument to PAPI by ddcc4 · Pull Request #17874 · Opentrons/opentrons

ddcc4 · 2025-03-25T15:53:07Z

Overview

This adds the new argument mm_from_edge to the touch_tip() public API function. AUTH-1625

The option is needed for PD protocols. And we generally think that it makes more sense for users to specify mm_from_edge instead of radius.

Support for mm_from_edge already exists in the Protocol Core and Engine (PR #17107), so all we need to do is to pass the new argument along to the Core.

We follow the same convention that we adopted in the Protocol Engine, that radius and mm_from_edge are mutually exclusive, so if the user specifies mm_from_edge, they must leave radius to its default value of 1.0

Test Plan and Hands on Testing

I added unit tests to check for all combinations of mm_from_edge and radius.

Review requests

Do I need to do anything else for API versioning? What API version would this go into?

Risk assessment

Medium. We're adding a new API option, and we have to support API changes forever once released.

jerader · 2025-03-25T16:44:17Z

            raise TypeError(f"location should be a Well, but it is {location}")

+        if mm_from_edge and radius != 1.0:
+            raise ValueError("radius must be set to 1.0 if mm_from_edge is specified")


jerader

make sense to me, definitely wait for Sanniti's review before merging though lol

SyntaxColoring

Seems like a good idea, thank you!

Do I need to do anything else for API versioning?

I'll defer to people on the authorship team, but I think so, yeah. Suggestions below.

What API version would this go into?

It would be API version 2.23 if you want this to be available in robot software v8.4.0. Otherwise 2.24, I think.

SyntaxColoring · 2025-03-25T17:50:21Z

        radius: float = 1.0,
        v_offset: float = -1.0,
        speed: float = 60.0,
+        mm_from_edge: float = 0,


We ought to prevent using this parameter on apiLevels that are too old.

Like, as written, someone can write a protocol that uses this new parameter and then share it with someone who's using an older robot software version. When the second person tries to run it, they'll get a confusing "invalid argument" error. One of the things that apiLevel is for is to let us turn that into a "you need to update your robot" error instead.

Here's an example of how we did that for the push_out param. That's an okay way of doing it, but we can do better by doing something like this:

mm_from_edge: float | _Unset = _Unset()

if not isinstance(mm_from_edge, _Unset) and api_version < ...: raise APIVersionError(...)

Where _Unset is this or something equivalent.

That avoids any hazard of conflating 0 with None, and it discourages the user from writing mm_from_edge=None and sneakily subverting the whole thing.

We should also add a .. versionchanged note to this reference docstring, and an entry on the versioning.rst page.

Thanks. I added the API version check and the versionchanged docstring.

I would prefer to keep the default as mm_from_edge=0 though:

It documents that the default value is indeed 0.

I think _Unset works best for features that can in principle be unset, but that's not the case for touch tip: we always have to touch tip at some distance from the well.

I'd prefer not to clutter up the type declarations. Right now, the argument is declared as a float, which is nice and simple (and also produces a warning in your editor if you try to call it with None like in your example). I'd like to avoid union types when possible.

Nothing else in instrument_context.py uses _Unset, even though many places in the code perform version checking, so I don't think I want to introduce an _Unset just for this one argument.

Hmm, I see @SyntaxColoring 's point about making mm_from_edge optionally _Unset.

I think _Unset works best for features that can in principle be unset, but that's not the case for touch tip: we always have to touch tip at some distance from the well.

True that we have to touch tip from some distance but when you're specifying a radius that's not 1, then your mm_from_edge value is not going to be 0 in reality. Same if the well is not a square or circle. In these cases an mm_from_edge of _Unset makes more sense.

Hey, after chatting with @sanni-t yesterday, I changed the mm_from_edge parameter to default to None instead of 0. Let me know what you think?

@SyntaxColoring Would you be OK with None? I can change it to _Unset, but I don't like how nothing else in this file uses it for version-gating (and _Unset makes the docstring slightly messier).

I think it's reasonable to use None over _Unset, and it's a matter of how you weigh the tradeoffs.

To elaborate on why I'd favor _Unset instead of None, None unfortunately lets the user write this:

requirements = {"apiLevel": "2.0"} # NOTE: old apiLevel! def run(context): ... pipette.touch_tip(..., mm_from_edge=None)

When the user test-runs this on their new robot software version, it will act as if they didn't specify any mm_from_edge value. OK. But when they share it out to someone who tries running it on an older robot software version, it will raise a potentially confusing TypeError because the mm_from_edge argument doesn't exist.

_Unset theoretically helps avoid that problem because the name _Unset more obviously signals that it's a private, internal thing, and because it's not publicly exported and not suggested by editor autocomplete.

I agree that it clutters up the docstring and that we should try to avoid that. Personally, to solve that, I would explore ways of tweaking the Sphinx output, and failing that, I might just let it lie.

Alright, I changed the default to _Unset().

I also found that if you define a __repr__ for _Unset, it generates plausible Sphinx docs, like this:

mm_from_edge: 'Union[float, _Unset]' = _Unset

which is probably good enough?

Oh, that __repr__() trick is a good find.

I like that. Some of our other method signatures are highly implausible 😅

ecormany · 2025-03-26T13:48:18Z

+        :param mm_from_edge: How far to move inside the well, as a distance from the
+                             well's edge.
+                             When ``mm_from_edge=0``, the pipette tip will move all the
+                             way to the edge of the target well. When ``mm_from_edge=1``,
+                             the pipette tip will move to 1 mm from the well's edge.
+                             Lower values will press the tip harder into the well's
+                             walls; higher values will touch the well more lightly, or
+                             not at all.
+                             ``mm_from_edge`` and ``radius`` are mutually exclusive: to
+                             use ``mm_from_edge``, ``radius`` must be unspecified (left
+                             to its default value of 1.0).


Very nice documentation! No notes. Unless you want your single line breaks to be paragraph breaks, in which case you need to make them double line breaks.

We should also add this to the bottom of the docstring:

.. versionchanged:: 2.24 Added the ``mm_from_edge`` parameter.

(I presume this is 2.24, since this PR is targeted on edge and 2.23 changes are going into chore_release-8.4.0)

codecov · 2025-03-28T19:52:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.91%. Comparing base (2644474) to head (ba9392d).

Additional details and impacted files

@@           Coverage Diff           @@
##             edge   #17874   +/-   ##
=======================================
  Coverage   61.91%   61.91%           
=======================================
  Files        2950     2950           
  Lines      227603   227602    -1     
  Branches    19190    19192    +2     
=======================================
  Hits       140912   140912           
+ Misses      86519    86518    -1     
  Partials      172      172

Flag	Coverage Δ
protocol-designer	`18.78% <ø> (ø)`
step-generation	`4.44% <ø> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…m legacy_instrument_core.

sanni-t

LGTM! Thanks for making the changes!

ddcc4 · 2025-04-03T15:10:04Z

@SyntaxColoring Hi! You had requested changes on this. I changed the default mm_from_edge from 0 to None, and version-gated the new arg. Would you be OK with this now?

SyntaxColoring

Looks great, thanks. Closing thoughts on None vs. _Unset here.

SyntaxColoring

TY!

# Overview This adds the new argument `mm_from_edge` to the `touch_tip()` public API function. The option is needed for PD protocols. And we generally think that it makes more sense for users to specify `mm_from_edge` instead of `radius`. Support for `mm_from_edge` already exists in the Protocol Core and Engine (PR #17107), so all we need to do is to pass the new argument along to the Core. We follow the same convention that we adopted in the Protocol Engine, that `radius` and `mm_from_edge` are mutually exclusive, so if the user specifies `mm_from_edge`, they must leave `radius` to its default value of 1.0 ## Test Plan and Hands on Testing I added unit tests to check for all combinations of `mm_from_edge` and `radius`. ## Review requests Do I need to do anything else for API versioning? What API version would this go into? ## Risk assessment Medium. We're adding a new API option, and we have to support API changes forever once released.

# Overview This adds the new argument `mm_from_edge` to the `touch_tip()` public API function. The option is needed for PD protocols. And we generally think that it makes more sense for users to specify `mm_from_edge` instead of `radius`. Support for `mm_from_edge` already exists in the Protocol Core and Engine (PR #17107), so all we need to do is to pass the new argument along to the Core. We follow the same convention that we adopted in the Protocol Engine, that `radius` and `mm_from_edge` are mutually exclusive, so if the user specifies `mm_from_edge`, they must leave `radius` to its default value of 1.0 ## Test Plan and Hands on Testing I added unit tests to check for all combinations of `mm_from_edge` and `radius`. ## Review requests Do I need to do anything else for API versioning? What API version would this go into? ## Risk assessment Medium. We're adding a new API option, and we have to support API changes forever once released. (cherry picked from commit 63f1512)

Support touch_tip(mm_from_edge) argument.

f8edb42

ddcc4 requested review from jerader and sanni-t March 25, 2025 15:53

ddcc4 requested a review from a team as a code owner March 25, 2025 15:53

Fix lint

8c37e1f

jerader reviewed Mar 25, 2025

View reviewed changes

jerader approved these changes Mar 25, 2025

View reviewed changes

SyntaxColoring requested changes Mar 25, 2025

View reviewed changes

ecormany reviewed Mar 26, 2025

View reviewed changes

ddcc4 added 2 commits March 28, 2025 15:46

Add version check for mm_from_edge.

019a705

Merge remote-tracking branch 'origin' into mm_from_edge

3dcf358

ddcc4 added 4 commits March 28, 2025 16:21

Disable C901 is too complex lint error.

bc1097e

This won't be available until API 2.24. And need to hide argument fro…

0996336

…m legacy_instrument_core.

Merge remote-tracking branch 'origin' into mm_from_edge

0d7d3d3

Use None instead of 0 as the default for mm_from_edge.

ba9392d

sanni-t approved these changes Apr 2, 2025

View reviewed changes

SyntaxColoring approved these changes Apr 9, 2025

View reviewed changes

ddcc4 added 4 commits April 9, 2025 14:18

Use _Unset() instead of None for default arg value.

568c47f

Merge remote-tracking branch 'origin' into mm_from_edge

437836f

Lint

f051ae2

More lint

c8cb926

SyntaxColoring approved these changes Apr 9, 2025

View reviewed changes

ddcc4 merged commit 63f1512 into edge Apr 9, 2025
24 checks passed

ddcc4 deleted the dc-mm_from_edge branch April 9, 2025 19:01

ddcc4 mentioned this pull request May 17, 2025

Candidate chore_release-8.5.0 cherry-pick branch #18379

Merged

Conversation

ddcc4 commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Test Plan and Hands on Testing

Review requests

Risk assessment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerader left a comment

Choose a reason for hiding this comment

Uh oh!

SyntaxColoring left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddcc4 Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sanni-t left a comment

Choose a reason for hiding this comment

Uh oh!

ddcc4 commented Apr 3, 2025

Uh oh!

SyntaxColoring left a comment

Choose a reason for hiding this comment

Uh oh!

SyntaxColoring left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ddcc4 commented Mar 25, 2025 •

edited

Loading

ddcc4 Apr 9, 2025 •

edited

Loading

codecov Bot commented Mar 28, 2025 •

edited

Loading