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

Feature/refactor chain processor #159

Closed

Conversation

Iyadhfaleh
Copy link
Contributor

I did some refactor and renaming method
I see that when we want to process messages with a chain we invoke the method with two arrays contains the same variable ($chain = new Chain($platform, $llm, [$processor], [$processor]);)

I changed the signature of this method

public function call(MessageBag $messages, array $options = []): ResponseInterface

To be like

public function process(MessageBag $messages, array $options = [], ?ChainAwareProcessor $chainProcessor = null): ResponseInterface

The $chainProcessor will have the outputProcessors and inputProcessors and will be used in need .

@Iyadhfaleh Iyadhfaleh force-pushed the feature/refactor-chain-processor branch from 4cc95eb to 9095ddb Compare December 16, 2024 19:44
@chr-hertel
Copy link
Member

Hey @Iyadhfaleh, thanks for your thoughts and proposals, let's break them down:

1. Combining processors and pumping up interface & trait

i agree that those processors parameters are often times quite similar and that we could completely combine that into a single interface or similar. but they do have a different semantic purpose - http communication vs data conversion.
still it is possible to implement both interfaces in one class - which is handy for slim implementations and leads to injecting the same instance into the same arguments. this is a drawback of the flexibility i intend to have with those two extension points. if you want to have less redundancy here, you could completely split the interface implementation in your processors. on top i only design interfaces and traits to have the least amount of methods needed to satisfy their users. there is no code using the added methods - so no value in having them atm.

2. Moving processors from constructor to process method

this is basically a violation to my understanding of that method and the state of the chain. processors are part of the chain state - intended that way by design. processors are considered more static instances of a configured chain, and DTO + minor options are the main arguments on a call to chain. in my understanding processors would not vary on multiple call to the same chain.

3. Renaming of Chain::call to Chain::process

this is the most opinionated change i guess, but still i get the idea since we're using processors internally in the chain. in my understanding within the chain we process input and output but the main deal is calling the platform. i just favor it this way.

So all in all I'm not really convinced about the changes + examples like examples/structured-output-clock.php are failing.
For future PRs, please make sure to isolate changes in separate PRs, so we can have an isolated discussion and potential integration of single ideas. thanks for taking the time!

@chr-hertel chr-hertel closed this Dec 16, 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