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

Proposal/WIP: action interface refactoring #331

Closed
wants to merge 5 commits into from
Closed

Conversation

pavelgj
Copy link
Collaborator

@pavelgj pavelgj commented Jun 6, 2024

The problem I'm trying to solve is that the actions defined and registered in the registry are not the same thing as what is being returned to the user. Actions are getting rewrapped in generator interface which creates two tiered type system and unnecessary steps when going from one to the other.

This refactoring proposal (demonstrated on Generator but applies to all other action types) defines a single type definition for GeneratorAction which is being stored in the registry and being returned by LookupGeneratorAction and passed into ai.Generate, etc. This makes LookupGeneratorAction very lightweight.

@pavelgj pavelgj requested a review from jba June 6, 2024 15:15
@jba
Copy link
Contributor

jba commented Jun 6, 2024

I see at least three things going on here:

  1. Streaming arg should be GenerateResponseChunk, not Candidate. Seems independent of this. Will fix in a separate PR.

  2. Make generator, etc. a smaller wrapper around action. Got it, can do. I assume you want to drop DocumentStore or any wrapper around the pair of index and retrieve functions, and just have them as two separate actions. Is that correct?

  3. Change Action from a concrete type to an interface. Why is this needed?

@jba
Copy link
Contributor

jba commented Jun 6, 2024

Actually I'm not going to fix s/Candidate/GenerateResponseChunk/ just yet because it will result in a merge mess with #326, so let's resolve that first.

@jba
Copy link
Contributor

jba commented Jun 6, 2024

Just reading the JS again... you have something like

type ModelAction = Action[GenerateRequest, GenerateResponse]

which means the same thing as in Go: ModelAction is just another name for the type on the right-hand side.
Why don't we just do the same thing in Go? We'd lose the Generate method on Model, but you don't have that either, so no great loss.

@pavelgj
Copy link
Collaborator Author

pavelgj commented Jun 6, 2024

Just reading the JS again... you have something like

type ModelAction = Action[GenerateRequest, GenerateResponse]

which means the same thing as in Go: ModelAction is just another name for the type on the right-hand side. Why don't we just do the same thing in Go? We'd lose the Generate method on Model, but you don't have that either, so no great loss.

I know enough Go to be dangerous... I'm at peak dangerous! :)
Yes, what you say makes sense and seems better.
I was typing a long response to the other comment, but I don't think it was very coherent, so I gave up.

My main motivation is make sure dynamic registry lookup is super efficient with minimal overhead.

@pavelgj pavelgj closed this Jul 9, 2024
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.

2 participants