php-llm / llm-chain

PHP library for building LLM-based features and applications.
MIT License
12 stars 3 forks source link

feat: introduce chain awareness in chain processors to nest them #118

Closed chr-hertel closed 1 month ago

chr-hertel commented 1 month ago

Changes:

image

OskarStark commented 1 month ago

From what I can see, this PR looks promising, but I am not that deep into the chain thing 😄 Maybe add some more unit tests before merging? I can support you if you want

chr-hertel commented 1 month ago

I'm so lazy about adding tests because I'm still unsure about the OOP slicing - this is why i currently mostly rely on the "functional" tests a.k.a. examples

chr-hertel commented 1 month ago

still flip flopping between events and processors - what do you prefer?

those processors are explicit but the event dispatcher is super common in symfony as extension point

OskarStark commented 1 month ago

Looking at this PR, I feel the chain thing is quite "complex" or "complicated". My feeling is that events will bring up the same results, while the code is less cluttered.

OskarStark commented 1 month ago

Double checked the events PR and now I am not sure anymore if events will really result in better code 🤔 What is your feeling?

chr-hertel commented 1 month ago

since i'm moving away from return values and using those input/output objects and calling all instances of processors the only difference from mechanical point of view is the missing EventDispatcher in between.

this is more like the Serializer component with encoder, normalizers, etc - so explicit extension points.

only if we end up having more events across the layers (chain, model, platform) later and implementations on top of the extension points will leverage more than those two here, there is a benefit in events due to the ease of use for implementation - plus symfony devs being familiar with events and better debugging

chr-hertel commented 1 month ago

this is why i would delay the decision for events and go with this implementation first - but wanted to double check your poc implementation again

chr-hertel commented 1 month ago

but yes, similar to that serializer/normalizer thing it get's confusing/complex by stacking those capabilities

chr-hertel commented 1 month ago

so, i think with an custom output processor you could with a additional logic like response instanceof ToolCallReponse or similar inject the additional message that your poc has.

this was one example of mine with a stupid processor just always adding on in and out: image

OskarStark commented 1 month ago

Should be part of the docs in the README

OskarStark commented 1 month ago

Should be part of the docs in the README

chr-hertel commented 1 month ago

Should be part of the docs in the README

:100: the "docs" are s**t right now

OskarStark commented 1 month ago

We are under heavy development, so its fine to have shitty docs 😄