-
Notifications
You must be signed in to change notification settings - Fork 190
feat(opentrons-ai-client,-opentrons-ai-server): generate PD protocol #18146
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(opentrons-ai-client,-opentrons-ai-server): generate PD protocol #18146
Conversation
8d113d0 to
7043f22
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #18146 +/- ##
==========================================
- Coverage 57.48% 57.48% -0.01%
==========================================
Files 3043 3043
Lines 255276 255275 -1
Branches 30517 30516 -1
==========================================
- Hits 146756 146755 -1
Misses 108333 108333
Partials 187 187
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
4c50455 to
eca8934
Compare
opentrons-ai-client/src/resources/utils/createProtocolUtils.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pushing this forward 🙌 A few thoughts on the current logic around determining whether the session is a Protocol Designer session or a Python Protocol session:
- We need to make this decision logic more explicit and structured, formalizing it in the API.
- The branching logic for "is this a Protocol Designer session?" should live outside the
fast.pyhandler. Ideally, the handler should only:- Accept and validate incoming message shapes
- Pass incoming data/requests off to dedicated processors
- Return the data to the user
Actionable Proposals
- Find a way to determine from the data in the API request what type of session this is instead of inferring from the history.
- Extract the session type resolution logic out of
fast.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@y3rsh Thanks for this awesome comments. I pushed the changes. Please have a look if I am on the right path. Thanks!
| first_message_content: Optional[str] = None | ||
| if body.history and len(body.history) > 0: | ||
| first_message = body.history[0] | ||
| # Assuming message structure is dict-like, check 'content' key | ||
| if isinstance(first_message, dict): | ||
| content_value = first_message.get("content") | ||
| if isinstance(content_value, str): | ||
| first_message_content = content_value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This chunk of code can be simplified in a couple ways. First off, if body.history will return True only if it is not None and it's not an empty list. Furthermore, according to the documentation I'm reading, "content" is a required field and is set to be a str, so the additional checks of types is unnecessary here. So we could have something like
first_message_content: Optional[str] = None
if body.history:
first_message_content = body.history[0]["content"]
Now if for some reason "content" is not guaranteed, keeping it as .get("content") would be fine, as that returns None if the key doesn't exist which is the default value to begin with. And if they history objects weren't guaranteed to be dictionaries, using a try/except catching TypeError would be another one of simplyfing this.
Furthermore, if the suggestion above is causing mypy errors, sometimes we use assert in the code to silence those, but assert should only be used for internal errors and not anything user facing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this complex only because of mypy :)
| # Determine protocol option based on content | ||
| protocol_option = "update" # Default | ||
| if first_message_content and "Write a protocol using" in first_message_content: | ||
| protocol_option = "create" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple things here. First off, if first_message_content is an empty string it will be False here, which seems to be fine in this case but just something to be aware of, as sometimes is None is the more accurate check. This can also be simplified to be a ternary statement, like
protocol_option = "create" if first_message_content and "Write a protocol using" in first_message_content else "update"
The other thing is using string values like this is a bad code smell since it's very easy for typos to sneak in. I'd either refactor this to be an Enum or a boolean if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jbleon95 I did some changes, please have a look. Thanks!
3d001ad to
0a9187c
Compare
opentrons-ai-client/src/organisms/ProtocolFormatSection/index.tsx
Outdated
Show resolved
Hide resolved
e521085 to
bdf9aba
Compare
bdf9aba to
b512e89
Compare
eb455eb to
2ce661c
Compare
2ce661c to
f6d080c
Compare
jbleon95
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more minor Python nitpicks but the Python code otherwise looks better with the recent changes
| user_id: str, | ||
| prompt: str, | ||
| history: Optional[List[Any]] = None, | ||
| protocol_format: Optional[ProtocolFormat] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the arguments listed as optional don't seem like they are optional where they're used in the code. Maybe I'm missing something but if they can't be None, do let that be a return type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A good point.
- This is a generic function and used in several places. For
update_protocol,protocol_formatis passed asNonesince it is not used. historycan also beNone: used onlycreate_protocolandupdate_protocol, while not used increate_chat_completion.
Overview
Closes AUTH-1599
Write a protocol ... API v2 ...because PD protocol is nothing to do with PAPIProtocol FormatWith PAPI

With PD

With ChatDisplay

Review requests
Risk assessment
Medium