xintongsong commented on PR #852: URL: https://github.com/apache/flink-agents/pull/852#issuecomment-4886258770
Hi @purushah , Sorry for the late response. I was busy preparing the 0.3 release, Flink Forward Asia, and catching up on a few other works, and only had the chance to carefully look into this PR today. First of all, thank you again for the contribution! I think this is a very useful and interesting feature, and the implementation quality is also very high. It’s clear that you’ve put a lot of thought and effort into both the design and the implementation. That said, for a feature of this importance and complexity, I would generally prefer to discuss the overall design first (e.g., through a GitHub Discussion) before moving into a full implementation. After going through the PR, I realized that most of my questions are actually about the design rather than the implementation itself. If we can align on these design questions first, we can hopefully avoid unnecessary rework later. --- Here are my current thoughts. ### 1. Should ChatModelRouter be modeled as a special kind of ChatModelSetup? I’m not entirely convinced yet. The biggest advantage of the current design is exactly what you pointed out: it requires almost no framework changes, and the router can be used just like any other ChatModelSetup, which makes the implementation straightforward. However, I also see some potential drawbacks. - The first one is observability. Today, EventLog is our primary observability mechanism. When using a normal ChatModelSetup, every model invocation is represented by ChatRequestEvent and ChatResponseEvent, so the model inputs and outputs are fully captured. With ChatModelRouter, especially when using an LLMRoutingStrategy, the model invocation used to determine which candidate should be selected is no longer represented in the EventLog. As a result, users cannot observe the routing decision process itself. - Another concern is metrics. Today, many model-related metrics (for example, input/output token usage) are naturally aggregated at the ChatModelSetup level. If a single ChatModelSetup internally invokes multiple different models, those metrics will all be mixed together, making it difficult to understand which model consumed which tokens. This could make cost analysis and cost control more difficult. One alternative direction (just as an example—I haven’t fully thought through the design yet) would be to treat model routing as a framework capability rather than a specialized ChatModelSetup, for example by introducing a dedicated ChatModelRouter resource type. My intuition is that this might naturally address some of the observability and metrics concerns mentioned above. That said, this is only an initial thought, not a concrete proposal. ### 2. The boundary between Public API and Internal Implementation doesn’t seem very clear yet This was another impression I had while reading through the PR. At the moment, most of the newly introduced concepts and implementations live inside the API module, so it’s difficult to immediately distinguish: * Which concepts are intended to be part of the public API that users should interact with. * Which ones are merely implementation details used internally by the framework. Ideally, I think the API module should contain only the minimal set of interfaces that users actually need to interact with. This would help us maintain API compatibility more easily across future releases while still giving us enough flexibility to evolve the internal implementation. ### 3. What is the intended user experience of this feature? I also think there is still room to discuss the user-facing API design. In other words, it may be helpful to first step back and think about how users are expected to use this feature end-to-end, and then evaluate whether the API naturally supports that workflow. For example: * Is a RulBasedRoutingStrategy actually necessary? I’m not entirely sure that keyword-based routing has enough real-world demand to justify exposing it as part of the public API. * Should caching be exposed as a general configuration option instead of a separate strategy? The current design, where CachingStrategy delegates to another strategy, looks like a perfectly reasonable internal implementation. However, from a user’s perspective, it might be more natural to simply enable or disable caching through configuration instead of explicitly wrapping one strategy with another. * Both RoutingStrategy and FallbackPolicy are currently exposed as public interfaces. Do we really expect users to implement these interfaces directly? Or would it be sufficient for most users if we simply provided a set of built-in implementations and let users choose among them through configuration? I'm not saying that I’m against the current design. In fact, I don’t have firm answers to the above questions either. What I'm trying to say is that, in order to answer these questions, we probably should have a more fundamental design discussion first: * How do we expect users to use this feature? * Which decisions should users make explicitly? * Which decisions can be handled by the framework? * For the decisions that do need to be exposed, are we exposing them through an API that is as simple and intuitive as possible? --- Overall, I think this is a valuable feature, and I really appreciate the amount of work you’ve put into this PR. My suggestion would be to treat this PR as a prototype for now rather than moving directly into detailed code review. As a next step, I think it would be helpful to first flesh out the overall design—either by opening a GitHub Discussion or by adding a design proposal to this PR—especially around the following questions: * What should the public API look like? * What should remain internal implementation details? * What is the intended user experience of this feature? Once we have aligned on those aspects, I believe the implementation details will become much easier to converge on, and hopefully we don't need to rework this PR a lot. WDYT? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
