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

docs(protocol-designer): proposal for adding Python to step generation #17330

Open
wants to merge 3 commits into
base: edge
Choose a base branch
from

Conversation

ddcc4
Copy link
Contributor

@ddcc4 ddcc4 commented Jan 22, 2025

This is my proposal for how we can add Python generation to step-generation while keeping most of the code structure as-is, minimizing massive rewrites.

(The TypeScript files are not part of this PR. I just included them to demonstrate to myself that this approach would work in real life.)

@ddcc4 ddcc4 requested review from shlokamin and jerader January 22, 2025 20:21
@ddcc4 ddcc4 requested a review from a team as a code owner January 22, 2025 20:21

When the reducer runs, it strings together all the non-empty JSON `commands` to get the final JSON output, and it'll string together all the non-empty `python` commands to get the final Python output.

We need one more tool to make this work: because the Python `mix()` command replaces both the aspirate and dispense, we need to _suppress_ Python generation from aspirate and dispense. We'll do that by adding a new flag to `curryCommandCreator`, so the final sequence becomes:
Copy link
Member

Choose a reason for hiding this comment

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

we could also have new command creators for commands that are sometimes used to generate json but not python that emit empty python strings - the converse of pythonOnlyMix, so I guess jsonOnlyAspirate.

Copy link
Contributor Author

@ddcc4 ddcc4 Jan 22, 2025

Choose a reason for hiding this comment

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

we could also have new command creators for commands that are sometimes used to generate json but not python that emit empty python strings - the converse of pythonOnlyMix, so I guess jsonOnlyAspirate.

Hm, I'm worried that that could lead to a lot of duplicate code between the existing aspirate and jsonOnlyAspirate.

There's another approach I considered, but I don't know if you would consider it too intrusive: We could add a generatePython flag to the Args that are passed to the CommandCreators. This feels intrusive because generatePython isn't logically part of the step parameters, but I think it would work. (Adding generatePython to the Args is also messier because some of the Args classes don't share a common ancestor, so I'd have to add the flag in lots of places.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Of these approaches, I sort of prefer suppressPython. I agree that a jsonOnlyAspirate could lead to a lot of duplicate code.

Copy link
Member

Choose a reason for hiding this comment

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

I think that if you factored out the common parts of jsonOnlyAspirate and aspirate you wouldn't have that much duplicate code

@ddcc4 ddcc4 force-pushed the dc-pd-step-gen-doc branch from 88af261 to f79364e Compare January 23, 2025 17:18
@ddcc4 ddcc4 force-pushed the dc-pd-step-gen-doc branch from f79364e to 73c2b72 Compare January 23, 2025 23:08
Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

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

The doc is thorough and this makes a lot of sense to me. I'm excited about this! Seems like a logical next step is mapping out the api mix(), transfer(), consolidate(), and distibute() command args and when the compound command would issue individual api commands vs one of those api commands.

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.

3 participants