-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Introduce Transformations Conditions #2382
Introduce Transformations Conditions #2382
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2382 +/- ##
===========================================
- Coverage 38.14% 17.31% -20.83%
===========================================
Files 78 82 +4
Lines 7865 8114 +249
Branches 1683 1728 +45
===========================================
- Hits 3000 1405 -1595
- Misses 4615 6646 +2031
+ Partials 250 63 -187
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@WaelKarkoub what do you think of this structure? |
@giorgossideris I believe I understood the intent behind this PR. Something I learned very recently is that abstraction can seem appealing, but it often introduces complexity and coupling that can escalate quickly (I have PRs to prove that.) I was working on audio support for autogen by integrating TTS and speech-to-text APIs, here are the two implementations: #2220 and #2098. Which implementation do you think is more enjoyable to use and, more importantly, extend? The second thing is the concept behind message transformations #1923 aligns with something called linear transformations. This approach involves using distinct matrices, each have a specific, single function like rotation or scaling—and combining them effectively. Our protocol implementations should act as specialized matrices, good at singular tasks like managing token limits or compressing data. In the case of this PR, simply adding an argument to the protocol implementors' constructors is completely fine. It might look like repeated code, and it is, but it's perfectly fine. Consider a scenario if we had to implement a new transformation that requires a new way of checking a condition. This going to force us to change the protocol (a breaking change) and refactor every single transformation, not just the ones supported by autogen but also the ones made by the community. What I liked about your first PR is that every single transformation is expected to make a change, and every single user expects those changes to be logged somewhere, so changing the protocol made perfect sense. I hope this clarifies my perspective. Also I'm on discord with the same handle if you would like to discuss it more |
Closing this PR, because it causes breaking changes to the contract. The intended functionality can be achieved by creating new |
Why are these changes needed?
Introduces the option to set a specific condition to be checked in order for a transformation to be applied.
Example: a user may want to apply a
MessageHistoryLimiter
transformation only after the token limit of the model has been reached.Related issue number
Closes #2306.
Checks