Fix 9990 - Rationalize ISummaryConfiguration & ISummaryRuntimeOption #10047
Conversation
github-actions
bot
added
area: runtime
|
I'd think that packages/test/snapshots/content should not be changed. In reply to: 1109910732 In reply to: 1109910732 In reply to: 1109910732 |
|
I will revert it - as it changed locally, I thought it would need to be checked in. In reply to: 1109910732 |
github-actions
bot
added
the
area: dds
|
This needs a note in BREAKING.md. In reply to: 1113575560 |
| * Flag that will enable changing elected summarizer client after maxOpsSinceLastSummary. | ||
| * This defaults to false (disabled) and must be explicitly set to true to enable. | ||
| */ | ||
| summarizerClientElection: boolean; |
There was a problem hiding this comment.
We can move it into separate issue (assuming I'm right).
But I'm not sure I see a point of these two properties above if heuristics are disabled, and we only can do on-demand summaries (at least that's how it looks from code). If that's true, then I'd expect a caller (i.e. outside process) fully controlling it, and making its own decisions which clients (and how often) Ok to run summaries. And probably by virtue of that - controlling lifetime of summarizer client as well.
I think this mode was built for Word specifically (or any other scenario that wants to run summaries on dedicated client in service). I'd think the right direction here - remove the overhead of needing two clients (main + summarizer) and have a way for Word to directly create summarizer client and controlling it directly, removing whole client that is not needed. I think this workflow is possible, and in such case these options should not be needed.
Let's follow up on that separately.
| @@ -226,7 +221,7 @@ export class Summarizer extends EventEmitter implements ISummarizer { | |||
| private async start( | |||
| onBehalfOf: string, | |||
| runCoordinator: ICancellableSummarizerController, | |||
| options?: Readonly<Partial<ISummarizerOptions>>): Promise<RunningSummarizer> { | |||
| newConfig?: ISummaryConfiguration): Promise<RunningSummarizer> { | |||
There was a problem hiding this comment.
I'm not sure why overwrite (in the form of newConfig) is even needed here. We are getting full settings in constructor - they should be exactly same settings we would get here. If so, I believe we should remove it.
I think the reason it was different in the past is that we were passing only "core" settings in ctor - ISummaryConfiguration, which dd not include ISummarizerOptions. Now we properly merged all settings into one structure, and the need to pass data piecemeal is gone.
There was a problem hiding this comment.
hahaha Vlad, it used to be a ISummarizerConfig -> DisableHeuristics argument and I had moved to a boolean. You asked to move to a config :)
The containerruntime exports summarizeOnDemand that ends up calling summaryManager.summarizeOnDemand
That will spawn a summarizer with Heuristics disabled.
There was a problem hiding this comment.
I will follow-up in a different issue - for now I'm removing the argument but we might still have a problem if I have my config enabled, the summarizer is not running and we try to run a summarizer on demand. I will clarify on the issue.
github-actions
bot
added
the
breaking change
# Conflicts: # packages/runtime/container-runtime/src/runningSummarizer.ts # packages/runtime/container-runtime/src/summarizerHeuristics.ts # packages/runtime/container-runtime/src/test/summarizerHeuristics.spec.ts
…zer start - will follow-up on a different issue.
#10047 Introduced changes to summary heuristics. This PR is some cleanup around old unused variables.
Draft for fixing #9990