feat: Support orchestration prompt module fallback (with stream)#1470
feat: Support orchestration prompt module fallback (with stream)#1470davidkna-sap wants to merge 33 commits intomainfrom
Conversation
1708ce5 to
7234bf0
Compare
4d2086d to
63c3943
Compare
63c3943 to
27cd0df
Compare
Co-authored-by: hyperspace-insights[bot] <209611008+hyperspace-insights[bot]@users.noreply.github.com>
Co-authored-by: hyperspace-insights[bot] <209611008+hyperspace-insights[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Summary
The pull request adds streaming support for module fallback configurations in the orchestration service. The implementation is generally well-structured with comprehensive test coverage. I identified one typo that has been addressed. The code properly handles streaming with fallback configs, validates stream options arrays, and merges intermediate failures from multiple config attempts.
PR Bot Information
Version: 1.17.50 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- Event Trigger:
issue_comment.created - Correlation ID:
92e3e530-01c5-11f1-8168-561f032297a5 - LLM:
anthropic--claude-4.5-sonnet
SummaryThe following content is AI-generated and provides a summary of the pull request:
feat: Support orchestration prompt module fallback with streamingNew Features✨ Streaming Support for Module Fallback: Extended orchestration module fallback to support streaming requests, allowing automatic failover between model configurations during stream operations. ChangesThis PR completes the streaming implementation for orchestration prompt module fallback, building on the non-streaming support added in #1454. Core Implementation:
Type Definitions:
Testing:
Sample Code:
GitHub Issues
Related Pull Requests
PR Bot InformationVersion:
|
Co-authored-by: hyperspace-insights[bot] <209611008+hyperspace-insights[bot]@users.noreply.github.com>
ff8ede9 to
42bef03
Compare
Co-authored-by: KavithaSiva <kavitha.sivakumar@sap.com>
KavithaSiva
left a comment
There was a problem hiding this comment.
Initial review done, with few refactoring requests.
There was a problem hiding this comment.
[req] This function is currently difficult to read with multiple cases tackled together for single/multiple module config and stream options.
Could you please refactor the function to have cleaner separations for each of these different cases?
…-streaming * origin/main: feat: Support orchestration prompt module fallback (#1454) v2.7.0 chore: Fix pnpm-lockfile refresh merge condition handling (#1497) feat: Add `rawResponse` property to `OrchestrationStreamResponse`& `getRequestId`-helpers (#1464) chore: Combine ai client type headers (#1545) chore(deps-dev): Bump @sap/cds-dk from 9.7.1 to 9.7.2 (#1548) chore: update pnpm transitive dependencies (#1544)
There was a problem hiding this comment.
The pull request adds streaming support for orchestration module fallback configurations. I found a typo and a logic issue where output filtering warnings are not issued when using array stream options. The refactoring suggestion from earlier comments has been partially addressed, though the function remains complex.
PR Bot Information
Version: 1.17.61 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- LLM:
anthropic--claude-4.5-sonnet - Event Trigger:
issue_comment.created - Correlation ID:
1b2b8f70-0b43-11f1-8660-bf77760f4759
Co-authored-by: hyperspace-insights[bot] <209611008+hyperspace-insights[bot]@users.noreply.github.com>
There was a problem hiding this comment.
[suggestion] I was brainstorming. Instead of providing an array of stream options what about having a type like this:
interface StreamOptionsWithOverrides {
global?: GlobalStreamOptions; // Top level only
default?: Omit<StreamOptions, 'global'>; // No global here
overrides?: Record<number, Omit<StreamOptions, 'global'>>; // No global here
} Which makes it simpler to provide global config and default config that has to fit all orchestration configs?
So, the usage looks cleaner:
// Array config - structured
const arrayClient = new OrchestrationClient([config1, config2, config3]);
arrayClient.stream(req, sig, {
global: { chunk_size: 100 },
default: { promptTemplating: { include_usage: false } },
overrides: {
0: { promptTemplating: { include_usage: true } }
}
});
// Array config - minimal (just global)
arrayClient.stream(req, sig, {
global: { chunk_size: 100 }
});
// Array config - just overrides
arrayClient.stream(req, sig, {
overrides: {
1: { outputFiltering: { overlap: 50 } }
}
}); Honestly, it would have been much easier if the stream options in module configuration had also been directly initialised in the constructor instead, a consideration for v3 maybe.
Another option would be to only allow global and default options for now and only allow overrides if there is enough demand.
What do you think of these suggestions?
There was a problem hiding this comment.
I do think this is cleaner, though as far as ease-of-use goes, simple cases can still use a single non-list StreamOptions as usual anyway. I mostly only implemented this more complicated way of doing this because the documentation/demos stressed the importance of each module being independent and very customised for each the module configuration.
That said, I will try implementing your suggestion to see how it looks.
There was a problem hiding this comment.
@davidkna-sap I would like to throw in a slight change to @KavithaSiva's idea: You could also change the API to be something like:
type StreamOptionsWithOverrides = StreamOptions & {
overrides?: Record<number, Omit<StreamOptions, 'global'>>; // No global here
}That would keep the stream options as is for the simple case and allow overrides for fallbacks.
There was a problem hiding this comment.
Wait a bit before implementing my suggestion. I discussed the API with @marikaner today and she also has some suggestions for the API. Let's discuss before we implement more.
|
@KavithaSiva Another thing I was considering: How you feel about removing the merging with the shared stream configuration? Currently it's not possible for e.g. a |
You mean |
2d6a95d to
4421dd3
Compare
4421dd3 to
a909126
Compare
Context
Closes https://github.com/SAP/ai-sdk-js-backlog/issues/461
What this PR does and why it is needed
Streaming-related part of #1454