mirror of
https://github.com/microsoft/agent-framework.git
synced 2026-06-16 21:04:09 +08:00
Document follow-up changes for the Magentic .NET sample
Agent-Logs-Url: https://github.com/microsoft/agent-framework/sessions/caa3488f-d6f5-494d-a928-a45d6a98b3c3 Co-authored-by: lokitoth <6936551+lokitoth@users.noreply.github.com>
This commit is contained in:
committed by
GitHub
Unverified
parent
ffb4a170fc
commit
b1d057e220
@@ -0,0 +1,113 @@
|
||||
# Proposed Changes to the Magentic .NET Sample
|
||||
|
||||
This document captures the changes recommended for `dotnet/samples/03-workflows/Orchestration/Magentic/Program.cs`
|
||||
following a deep review of the framework code that backs `MagenticWorkflowBuilder`,
|
||||
`MagenticOrchestrator`, `AIAgentHostExecutor`, and the `ChatProtocolExecutor` pipeline.
|
||||
|
||||
The sample compiles cleanly (`dotnet build` → 0 warnings, 0 errors), the workflow is wired correctly,
|
||||
and it should run end-to-end against a properly configured Azure AI Foundry project. The items below are
|
||||
either parity gaps with the Python original (`python/samples/03-workflows/orchestrations/magentic.py`)
|
||||
or small robustness improvements — none are blockers.
|
||||
|
||||
---
|
||||
|
||||
## 1. Final transcript only contains the manager's final answer (parity gap)
|
||||
|
||||
### Observation
|
||||
|
||||
`MagenticOrchestrator.PrepareFinalAnswerAsync` yields a single-element `List<ChatMessage>` containing
|
||||
only the manager's synthesized final answer
|
||||
(`dotnet/src/Microsoft.Agents.AI.Workflows/Specialized/Magentic/MagenticOrchestrator.cs:297-302`).
|
||||
The orchestrator is the only registered output executor (`MagenticWorkflowBuilder.cs:127`,
|
||||
`WithOutputFrom(orchestrator)`), and the per-turn `AgentResponseUpdate` values from the team agents
|
||||
bypass the output filter and surface only as streamed `AgentResponseUpdateEvent` instances
|
||||
(`InProcessRunnerContext.cs:244-256`).
|
||||
|
||||
The Python sample uses `intermediate_outputs=True` and prints the **full multi-agent transcript**
|
||||
at the end. The .NET sample's "Final Conversation Transcript" section therefore prints only
|
||||
the manager's summary, which is misleading given the section title.
|
||||
|
||||
### Recommended change
|
||||
|
||||
Accumulate the streamed agent text yourself during the run and print that as the transcript, instead
|
||||
of relying on the terminal `WorkflowOutputEvent`. Concretely:
|
||||
|
||||
- Maintain a `List<(string Speaker, StringBuilder Text)>` keyed off `ResponseId` change
|
||||
(the same boundary `WriteStreamingUpdate` already uses).
|
||||
- On each `AgentResponseUpdateEvent`, append `update.Update.Text` to the current speaker's buffer,
|
||||
using `updateEvent.Update.AuthorName ?? updateEvent.ExecutorId` as the speaker.
|
||||
- After the `await foreach` completes, print the accumulated buffers, followed by the manager's
|
||||
final answer captured from `WorkflowOutputEvent`.
|
||||
|
||||
This matches the Python sample's `outputs = cast(list[Message], output_event.data)` printout and
|
||||
makes the section title accurate.
|
||||
|
||||
## 2. Rename or repurpose the "Final Conversation Transcript" header
|
||||
|
||||
If we choose **not** to implement (1), rename the printed header to something accurate such as
|
||||
**"Final Answer (from MagenticManager)"** so users do not expect the per-agent turns there. The
|
||||
per-agent text is already streamed live as `- {executorId}: …` lines during execution.
|
||||
|
||||
## 3. `PauseIfInteractive()` blocks the workflow scheduler thread
|
||||
|
||||
### Observation
|
||||
|
||||
`Console.ReadLine()` is called synchronously from inside the `await foreach (WorkflowEvent …)` loop.
|
||||
Because that loop drives the workflow's superstep pump, blocking it pauses the entire workflow until
|
||||
the user hits Enter. This matches the documented intent ("block to allow user to read the plan
|
||||
before continuing"), and the Python sample does the same with `run_in_executor(None, input, …)`,
|
||||
so behavior is consistent. However:
|
||||
|
||||
- A long pause can exceed the lifetime of any externally injected `CancellationToken` and the user has
|
||||
no way to cancel from the prompt.
|
||||
- On CI the prompt is skipped via `Console.IsInputRedirected`, which is correct.
|
||||
|
||||
### Recommended change (optional polish)
|
||||
|
||||
Wrap the prompt in `await Task.Run(() => Console.ReadLine())` so the Read happens off the scheduler
|
||||
thread and is awaitable. This is a cosmetic robustness improvement and not required for correctness.
|
||||
|
||||
## 4. Document the disabled plan-signoff path
|
||||
|
||||
`RequirePlanSignoff(false)` means the orchestrator never raises a `RequestInfoEvent` for plan approval,
|
||||
so the sample legitimately does not handle one. Add a short comment near the
|
||||
`.RequirePlanSignoff(false)` call explaining that switching this to `true` would also require handling
|
||||
`RequestInfoEvent` in the event loop and replying via `run.SendResponseAsync(...)`. This keeps the
|
||||
sample self-explanatory for users who copy it as a starting point.
|
||||
|
||||
## 5. Consider explicitly logging `WorkflowWarningEvent`
|
||||
|
||||
`MagenticOrchestrator` and `MagenticManager` both surface non-fatal issues (e.g., progress-ledger
|
||||
JSON parse retries, empty next-speaker fallback) via `WorkflowWarningEvent`
|
||||
(`MagenticManager.cs:96`, `MagenticOrchestrator.cs:229`, `:263`, `:271`). The current sample
|
||||
silently ignores them. Adding a `case WorkflowWarningEvent warn: Console.WriteLine($"[warn] {warn.Message}");`
|
||||
arm to the `switch` would make the sample more useful for debugging when the model returns malformed
|
||||
JSON for the progress ledger.
|
||||
|
||||
## 6. No change required for the following items (validated)
|
||||
|
||||
- **Input typing**: `RunStreamingAsync(workflow, new List<ChatMessage> { … })` is accepted by the
|
||||
orchestrator's `ChatProtocolExecutor` route for `IEnumerable<ChatMessage>`
|
||||
(`ChatProtocolExecutor.cs:84-86`).
|
||||
- **TurnToken trigger**: required because `MagenticOrchestrator` sets `AutoSendTurnToken = false`
|
||||
(`MagenticOrchestrator.cs:92-93`); `emitEvents: true` correctly enables streaming for participants
|
||||
(`MagenticOrchestrator.cs:286`).
|
||||
- **Switch ordering**: `AgentResponseUpdateEvent` derives from `WorkflowOutputEvent`; matching it
|
||||
first is correct, and the typed `WorkflowOutputEvent when outputEvent.Is<List<ChatMessage>>()`
|
||||
arm only catches the orchestrator's terminal yield.
|
||||
- **Limit-exhausted path**: `RunCoordinationRoundAsync` (line 209-210) also yields a
|
||||
`List<ChatMessage>`, so `finalOutput` is populated even if max rounds/resets are hit.
|
||||
- **Code Interpreter tool**: `HostedCodeInterpreterTool` on the coder is supported by the Foundry
|
||||
Responses path; same pattern is used in `Agent_Step24_CodeInterpreterFileDownload`.
|
||||
- **`OPENAI001` suppression**: handled at the project level via `<NoWarn>` in `Magentic.csproj`.
|
||||
|
||||
---
|
||||
|
||||
## Suggested order of work
|
||||
|
||||
1. (1) Capture streamed updates and print the full transcript — biggest UX/parity win.
|
||||
2. (5) Surface `WorkflowWarningEvent`s — small change, large debugging value.
|
||||
3. (4) Add the plan-signoff comment — documentation only.
|
||||
4. (3) Off-thread the `Console.ReadLine` — cosmetic.
|
||||
|
||||
Item (2) is only needed if (1) is not done.
|
||||
Reference in New Issue
Block a user