- Issue created by @murz
- @murz opened merge request.
- π¦π²Armenia murz Yerevan, Armenia
I created two base classes:
- AiProviderRequestBaseEvent
- AiProviderResponseBaseEvent
that stores all the common properties and getters-setters for the child events - this allowed us to make the real event classes compact and almost empty: + 521 β 819 lines ;)Moving the issue to needs review.
- π©πͺGermany a.dmitriiev
a.dmitriiev β made their first commit to this issueβs fork.
- π©πͺGermany a.dmitriiev
The idea is good and the implementation looks also fine. I have only one concern, please check the MR comment.
- π¦π²Armenia murz Yerevan, Armenia
So, if the current implementation is okay, let's merge this then? Rebased on the fresh base branch, please make a fresh review.
- π©πͺGermany a.dmitriiev
It looks good now, but there is now problem in
src/Base/OpenAiBasedProviderClientBase.phpline 356, thesetChatTokenUsagemethod expects second parameter to be array, butStreamedResponseobject is passed. Maybe Marcus can check this?Because of this error there is no way to test the Streamed logging.
- π¦π²Armenia murz Yerevan, Armenia
Seems the problem was in the wrong order of parameters, I fixed this and reworked the code to use named parameters to minimize such errors in the future, take a look pls.
- π©πͺGermany marcus_johansson
I would say this looks good now and improves a lot, I will await @artem before settings to RTBC and merge.
- π©πͺGermany a.dmitriiev
The problem is gone now. Finally it is ready to be merged. RTBC'ed. Sorry for this issue ping-pong, but as this is a foundation for future logging system, it should be flawless as possible. Thank you @murz for your effort!
- π©πͺGermany marcus_johansson
Thank you all - this is getting merged now.
Now that this issue is closed, please review the contribution record.
As a contributor, attribute any organization helped you, or if you volunteered your own time.
Maintainers, please credit people who helped resolve this issue.
-
marcus_johansson β
committed 8366a30a on 1.2.x authored by
murz β
Issue #3540682: Simplify AI Event classes by using base classes
-
marcus_johansson β
committed 8366a30a on 1.2.x authored by
murz β