Python: Improve python sample validation workflow - #7350
Conversation
TaoChenOSU
added
python
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
github-actions
Bot
changed the title
agent-framework-automation
Bot
added
the
documentation
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
This PR improves the Python sample-validation pipeline by adding deterministic replay via cached “playbooks”, splitting/adjusting workflow jobs for stability, and fixing/modernizing several samples (notably around Foundry hosted-agent scenarios and Magentic orchestration output handling).
Changes:
- Add a cached playbook mechanism (record on success, replay on subsequent runs) and integrate it into the sample-validation workflow.
- Refine sample discovery (treat
main.py/app.pydirectories as a single sample; skip dot/venv-style dirs) and pass file-based skills into validation agents. - Update workflows/actions to restore/save playbooks via cache and restructure/disable some jobs temporarily; fix sample code/docs for hosted agents and orchestration output.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| python/scripts/sample_validation/workflow.py | Inserts the replay executor step into the validation workflow DAG. |
| python/scripts/sample_validation/replay_executor.py | New executor that replays cached playbooks and forwards remaining samples to agent validation. |
| python/scripts/sample_validation/playbook.py | New playbook model/store + hash/replay runtime for deterministic validation without invoking Copilot. |
| python/scripts/sample_validation/models.py | Extends config/results models to support caching, replay, and cached result propagation. |
| python/scripts/sample_validation/discovery.py | Updates discovery to treat main.py/app.py directories as a single sample and avoid dot/venv dirs. |
| python/scripts/sample_validation/create_dynamic_workflow_executor.py | Adds skills support for agents and records playbooks on successful validations. |
| python/scripts/sample_validation/run_dynamic_validation_workflow_executor.py | Ensures cached (replayed) results are included in final ExecutionResult. |
| python/scripts/sample_validation/main.py | Adds CLI knobs for playbook cache dir / disabling cache / agent timeout; prints cache status. |
| python/scripts/sample_validation/skills/foundry-hosted-agent-validation/SKILL.md | New skill documenting end-to-end validation process for Foundry hosted-agent samples + playbook constraints. |
| python/scripts/sample_validation/skills/foundry-hosted-agent-validation/scripts/validate_hosted_agent.sh | New automation script that runs native + azd-local + deployed hosted-agent validation and cleanup. |
| python/scripts/sample_validation/skills/foundry-config-setup/SKILL.md | New skill for converting placeholder Foundry config to env-driven configuration. |
| python/samples/04-hosting/foundry-hosted-agents/README.md | Adds “Most up to date vs Legacy” status legend and labels samples accordingly. |
| python/samples/04-hosting/foundry-hosted-agents/responses/06_files/main.py | Simplifies toolbox wiring by switching to FoundryToolbox. |
| python/samples/04-hosting/foundry-hosted-agents/responses/09_foundry_skills/README.md | Adds guidance recommending Foundry Toolbox MCP skills for production scenarios. |
| python/samples/04-hosting/foundry-hosted-agents/responses/using_deployed_agent.py | Modernizes hosted-session create/delete flow against project_client.agents APIs. |
| python/samples/03-workflows/orchestrations/magentic.py | Updates streaming output handling to separate manager output from participant streaming and print final output text. |
| python/packages/orchestrations/agent_framework_orchestrations/_magentic.py | Adds MagenticOrchestrator.MANAGER_NAME constant and uses it for the orchestrator’s executor id. |
| python/samples/autogen-migration/orchestrations/04_magentic_one.py | Aligns output handling with MagenticOrchestrator.MANAGER_NAME and simplifies final output printing. |
| python/samples/autogen-migration/single_agent/01_basic_agent.py | Fixes PEP 723 run command and adds python-dotenv dependency metadata. |
| python/samples/autogen-migration/single_agent/02_agent_with_tool.py | Adds PEP 723 metadata block (deps + run command). |
| python/samples/semantic-kernel-migration/processes/nested_process.py | Adds python-dotenv to PEP 723 metadata dependencies. |
| python/samples/semantic-kernel-migration/processes/fan_out_fan_in_process.py | Adds python-dotenv to PEP 723 metadata dependencies. |
| python/samples/semantic-kernel-migration/orchestrations/sequential.py | Adds python-dotenv to PEP 723 metadata dependencies. |
| python/samples/semantic-kernel-migration/orchestrations/magentic.py | Adds python-dotenv to PEP 723 metadata dependencies. |
| python/samples/semantic-kernel-migration/orchestrations/handoff.py | Adds python-dotenv to PEP 723 metadata dependencies. |
| python/samples/semantic-kernel-migration/orchestrations/group_chat.py | Adds python-dotenv to PEP 723 metadata dependencies. |
| python/samples/semantic-kernel-migration/orchestrations/concurrent_basic.py | Adds python-dotenv to PEP 723 metadata dependencies. |
| python/samples/semantic-kernel-migration/openai_responses/01_basic_responses_agent.py | Adds python-dotenv to PEP 723 metadata dependencies. |
| python/samples/semantic-kernel-migration/openai_responses/02_responses_agent_with_tool.py | Adds python-dotenv to PEP 723 metadata dependencies. |
| python/samples/semantic-kernel-migration/openai_responses/03_responses_agent_structured_output.py | Adds python-dotenv to PEP 723 metadata dependencies. |
| python/samples/semantic-kernel-migration/chat_completion/01_basic_chat_completion.py | Adds python-dotenv to PEP 723 metadata dependencies. |
| python/samples/semantic-kernel-migration/chat_completion/02_chat_completion_with_tool.py | Adds python-dotenv to PEP 723 metadata dependencies. |
| python/samples/semantic-kernel-migration/chat_completion/03_chat_completion_thread_and_stream.py | Adds python-dotenv to PEP 723 metadata dependencies. |
| python/samples/semantic-kernel-migration/copilot_studio/01_basic_copilot_studio_agent.py | Adds python-dotenv and clarifies SK vs AF dependency-generation incompatibility. |
| python/samples/semantic-kernel-migration/copilot_studio/02_copilot_studio_streaming.py | Adds python-dotenv and clarifies SK vs AF dependency-generation incompatibility. |
| .github/workflows/python-sample-validation.yml | Splits/adjusts jobs, adds playbook cache save steps, updates env wiring, and adds dedicated foundry-hosted-agent job. |
| .github/actions/sample-validation-setup/action.yml | Restores playbook cache and installs Python deps (via python-setup) as part of validation setup. |
| .github/actions/sample-validation-save-playbooks/action.yml | New composite action to save playbooks cache even when validation fails. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 82%
✓ Correctness
No actionable issues found in this dimension.
✓ Security Reliability
No actionable issues found in this dimension.
✓ Test Coverage
This PR adds substantial new infrastructure (playbook caching/replay in ~350-line playbook.py, snapshot/restore logic, modified discovery logic) to the sample_validation CI scripts without any unit tests. The sample_validation module has never had tests, so this isn't a regression, but the playbook replay logic in particular handles complex process management, timeout handling, and file integrity concerns that would benefit from focused testing. The only library code change (MagenticOrchestrator.MANAGER_NAME refactoring) is adequately covered by existing tests in test_magentic.py. This PR adds substantial new logic for cached playbook replay (playbook.py at ~346 lines, replay_executor.py at ~83 lines) and modifies the workflow executor, all without any unit tests. The sample_validation module has no existing test suite, but the new playbook module contains non-trivial logic (hashing, serialization, async subprocess management, process group teardown, file snapshot/restore) that would benefit from unit tests to catch regressions — especially since this code directly gates whether CI validates samples via an LLM agent or a cached script.
✓ Failure Modes
The PR is well-structured with solid failure handling in most paths. The snapshot/restore mechanism in the agent executor is correct, and the new playbook caching adds a sensible optimization layer. The CI workflow changes are straightforward (disabling jobs, renaming secrets/vars, adding cache save steps). The only concrete failure mode finding involves the replay path construction in playbook.py (part of chunk 2), where SampleInfo is reconstructed with an incorrect base path, silently disabling the snapshot/restore safety net during playbook replay. The new playbook replay system is well-structured with good process-group cleanup and snapshot restoration. One concrete failure mode exists: an unhandled exception from any single
replay_playbookcall (e.g., temp file creation failure) crashes the entire replay loop, causing all samples—both already-cached successes and remaining ones—to be lost rather than forwarded to the agent.
✗ Design Approach
One sample now deletes hosted-agent sessions using the local
AgentSession.session_idinstead of the service-issued hosted session ID, which breaks cleanup. Separately, the new directory-sample discovery rule stops descending as soon as a folder containsmain.py/app.py, which silently drops other documented runnable samples under that folder from validation. The replay-before-agent split is a good fit for the instability problem, but I found two design-level correctness gaps in the new cache layer: directory samples are hashed as only their.pyfiles even though the repo defines the sample as the whole folder, and replay reconstructs the sample path relative topython/instead ofpython/samples/, so the promised snapshot/restore logic does not target the real sample files.
Flagged Issues
-
python/scripts/sample_validation/playbook.py:107only includes*.pyin the cache key, but the new hosted-agent contract defines the sample as the entire folder (SKILL.md:97-106) andcompute_sample_hashitself says "any change to the sample invalidates the cached playbook" (playbook.py:114-117). Changes toagent.yaml,agent.manifest.yaml, orrequirements.txtwill therefore reuse a stale playbook and can silently skip revalidation. -
python/scripts/sample_validation/playbook.py:238rebuilds replay targets aspython_root / sample_path, even thoughSampleInfo.relative_pathis stored relative tosamples_dir(models.py:36-40) andsamples_dirispython_root / "samples"(__main__.py:124-126). That means replay snapshots/restorespython/04-...instead ofpython/samples/04-..., so any playbook that edits sample files can leave the repo dirty despite the replay contract atplaybook.py:232-235.
Suggestions
- In
replay_executor.py, thefor coro in asyncio.as_completed(...)loop at line 72 has no exception handling. Ifreplay_playbookraises (rather than returning a FAILURE RunResult)—possible iftempfile.NamedTemporaryFileat playbook.py:257 raises OSError before the try/finally block—the entirereplayhandler aborts without sending aReplayResult. All collectedcached_resultsandremainingsamples are lost, stalling the workflow. Wrappingawait coroin a try/except that treats unexpected exceptions as replay failures (appending the sample toremaining) would make this resilient.
Automated review by TaoChenOSU's agents
|
Flagged issue
Source: automated DevFlow PR review |
|
Flagged issue
Source: automated DevFlow PR review |
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 93%
✗ Correctness
The PR introduces playbook caching, sample discovery improvements, and several sample fixes. One high-severity bug exists:
delete_hosted_agent_sessionpassessession.session_id(a locally generated UUID) instead ofsession.service_session_id(the actual service-managed ID) to the delete API, which will fail to delete the hosted session server-side. The previous resolved review comment flaged a related issue but the code still uses the wrong ID. The magentic output handling changes are correct. The new playbook caching system and replay executor are well-structured. The main correctness issue is inreplay_playbookwhere the on-disk sample path is reconstructed incorrectly:playbook.samplestores a path relative tosamples_dir(python/samples/) but is joined withpython_root(python/), producing a non-existent path likepython/04-hosting/...instead ofpython/samples/04-hosting/.... This makes the snapshot/restore mechanism silently no-op, so a replay script that edits sample files will leave the repository dirty. This was previously flagged and marked resolved, but the diff still shows the unfixed code.
✗ Security Reliability
The
delete_hosted_agent_sessionfunction inusing_deployed_agent.pypassessession.session_id(a locally-generated UUID) to the server-sidedelete_sessionAPI, instead ofsession.service_session_id(the actual hosted-agent session identifier). This was introduced when refactoring the old code which correctly usedsession.service_session_id. The playbook replay system is well-structured overall with good process isolation and cleanup. However, the path reconstruction bug at playbook.py:238 (previously flaged and resolved but still present in this diff) means the snapshot/restore mechanism operates on incorrect paths, breaking the guarantee that replays never leave the repository dirty. No new security issues beyond the inherent design of executing cached agent-authored scripts.
✓ Test Coverage
This PR introduces substantial new infrastructure code for the sample validation system — playbook caching/replay (~350 lines in playbook.py), a replay executor (replay_executor.py), directory-based sample discovery (discovery.py changes), and snapshot/restore logic (create_dynamic_workflow_executor.py) — with zero unit test coverage. Grep across all test files confirms no file references
discover_samples,DiscoverSamplesExecutor, orsample_validation. While this is CI tooling rather than production code, several modules contain non-trivial logic (subprocess process-group management, file-system snapshot/restore, content-hash-based cache invalidation) that is straightforward to unit test and prone to subtle bugs. Thesample_validationmodule has historically had no tests, but the new code contains pure, easily-testable functions (hash computation, serialization round-trips, store load/save, slug generation) and async logic with meaningful edge cases (timeout handling, process cleanup, snapshot/restore). At minimum the coreplaybook.pyfunctions warrant unit tests to prevent regressions in the caching logic that now gates whether the LM agent is invoked at all.
✗ Failure Modes
The PR is a large workflow infrastructure improvement with playbook caching, job splitting, and sample fixes. Most changes are sound. One concrete failure-mode bug exists: the new
delete_hosted_agent_sessionhelper passessession.session_id(an auto-generated local UUID) to the server-side delete call instead ofsession.service_session_id(the real hosted-agent session identifier). This silently fails to clean up the remote session, leaking server-side resources. This was flagged in a prior review round as resolved, but the code still contains the bug. The path construction inreplay_playbook(playbook.py:238) reconstructs the sample's on-disk path incorrectly:playbook.samplestores a path relative tosamples_dir(i.e.,python/samples/), but line 238 joins it topython_root(python/), omitting thesamples/segment. This causes the snapshot/restore safety mechanism to silently target a non-existent path, so the snapshot is empty and the repo can be left dirty after a replay that edits sample files. This issue was previously raised in an earlier review round and marked resolved, but the code in the current diff remains unchanged.
✗ Design Approach
I found two design-level problems in the current chunk. One is a correctness bug in the hosted-agent cleanup sample: it now deletes sessions by the local
AgentSession.session_idinstead of the service-managed session id returned by Foundry, which can leak hosted sessions. The other is in the new directory-sample discovery rule: treating any folder withmain.py/app.pyas a terminal sample and stopping descent silently drops real child samples such as the multiple DevUI samples documented undersamples/02-agents/devui. The replay cache path reconstruction is still inconsistent with how samples are discovered and stored, so replay can miss the real sample tree when it snapshots/restores edits. Separately, the new hosted-agent validation script skips the native-runtime phase for every invocations sample even though the documented contract and repo sample matrix treat invocations samples as first-class hosted-agent samples that should pass all three validation phases.
Flagged Issues
- playbook.py:238 —
SampleInfo(path=python_root / sample_path, ...)constructs the wrong absolute path becausesample_path(fromplaybook.sample) is relative tosamples_dir(python/samples/), notpython_root(python/). This causes the snapshot/restore mechanism to silently target a non-existent directory, so replays that edit sample files leave the repository dirty. Previously flaged and marked resolved, but the current diff still contains the unfixed code. - using_deployed_agent.py:65 —
session.session_idis a locally generated UUID, not the service-managed session ID stored insession.service_session_id. The server-side hosted-agent session will never be deleted, leaking resources. Previously flagged and marked resolved, but the current diff still uses the wrong identifier.
Automated review by TaoChenOSU's agents
|
Flagged issue playbook.py:238 — Source: automated DevFlow PR review |
|
Flagged issue using_deployed_agent.py:65 — Source: automated DevFlow PR review |
GitHub Actions
Inactive
|
|
||
| # If the whole directory is a sample, add the directory itself and do NOT descend into | ||
| # it: everything under a main.py/app.py entry point belongs to that one sample. | ||
| if any(file in ("main.py", "app.py") for file in files): |
There was a problem hiding this comment.
What happens to sibling samples when a directory has a top-level main.py? samples/02-agents/devui has main.py next to in_memory_mode.py and a dozen sub-sample folders (agent_weather, workflow_fanout, ...); this branch collapses the whole tree into one sample, and the pruning plus continue silently drops the rest from discovery and the report. Should this only collapse when the directory has no other entry-point files, or at least log what was subsumed so coverage loss is visible?
| parser.add_argument( | ||
| "--playbooks-dir", | ||
| type=str, | ||
| default="./sample_validation/playbooks", |
There was a problem hiding this comment.
Should python/samples/sample_validation/playbooks/ be gitignored? The module docstring says playbook scripts are never committed, but nothing enforces that...
| try: | ||
| data = json.loads(path.read_text(encoding="utf-8")) | ||
| playbook = Playbook.from_dict(data) | ||
| except (OSError, ValueError, KeyError) as ex: |
There was a problem hiding this comment.
Could one malformed playbook take down the whole run? from_dict raises TypeError for a non-dict top level (data["sample"] on a list) and AttributeError for a non-dict env, neither of which is caught here. Should we broaden this to Exception to match the "ignore unreadable playbook" intent?
| local port="$1" pids | ||
| if command -v lsof >/dev/null 2>&1; then | ||
| pids="$(lsof -ti "tcp:${port}" 2>/dev/null || true)" | ||
| [ -n "$pids" ] && kill -9 $pids 2>/dev/null || true |
There was a problem hiding this comment.
Could this kill an unrelated process on a developer machine? free_port SIGKILLs whatever owns port 8088. It looks like this skill also covers local runs. If the sample's server never bound because the port was already taken by a user's own service, cleanup kills that service instead.
Shouldn't we limit the kill to the PID tree we spawned (SERVER_PID and descendants) and only warn when a "foreign" process holds the port?
eavanvalkenburg
left a comment
eavanvalkenburg
left a comment
There was a problem hiding this comment.
honestly, I'm not sure how I feel about using agents to do deterministic things, we should be able to validate the vast majority of samples using code, which will be faster and cheaper. Can we scope this to only samples that are more difficult to validate? And I also think we can think about validation more intelligently, for instance by first running static code checks to validate the code, then picking and choosing ones that are in paths that often break, like the most complex OAI Responses sample, run that, if good, no need to run all OAI Responses samples...
|
|
||
| validate-02-agents: | ||
| name: Validate 02-agents | ||
| if: false # Temporarily disabled - to free up Copilot quota for other jobs |
There was a problem hiding this comment.
what does copilot have to do with this?
| | ✅ | 6 | [Files](responses/06_files/) | An agent demonstrating how to work with files in a hosted agent session, including uploading files to a hosted agent session and having the agent read and manipulate those files at runtime. | | ||
| | ✅ | 7 | [Observability](responses/07_observability/) | A sample demonstrating how to enable observability for the agent deployed to Foundry. | | ||
| | ✅ | 8 | [Azure AI Search RAG](responses/08_azure_search_rag/) | An agent with Retrieval Augmented Generation (RAG) capabilities backed by Azure AI Search, grounding answers in documents indexed in a pre-provisioned search index. | | ||
| | ⚠️ | 9 | [Foundry Skills](responses/09_foundry_skills/) | An agent that uploads `SKILL.md` files to the Foundry Skills REST API and downloads them at startup, decoupling tone/policy guidelines from agent code. | |
There was a problem hiding this comment.
why do we have a sample that does not work, and even if that is temporary, we should not need a status column, because every sample should work, otherwise it shouldn't be in the repo, people might not look at readme's, so if there is something broken, if it is hard to validate then we should have notes inside the sample explaining that
| path: python/samples/sample_validation/reports/ | ||
|
|
||
| validate-autogen-migration: | ||
| name: Validate autogen-migration |
There was a problem hiding this comment.
can we not start skipping the migration samples, and just add language that the exact new shape of the AF side of those samples might be slightly different and we will maintain best effort, but do not run validation, seems wasteful for such static content
Motivation & Context
Addresses #7348
Description & Review Guide
Run result:
Related Issue
Fixes #7348
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.