Skip to content

Clean up samples, tests, and docs#619

Open
bmehta001 wants to merge 1 commit intomainfrom
bhamehta/nits
Open

Clean up samples, tests, and docs#619
bmehta001 wants to merge 1 commit intomainfrom
bhamehta/nits

Conversation

@bmehta001
Copy link
Copy Markdown
Contributor

Fix SDK:

  • JS: Fixed tool.function.description.trim() TypeError when description is undefined (chatClient.ts:170)

Fix samples/tests

  • C#: Added missing ToolCallId on tool response
  • Python: Removed unnecessary async/await (6 files), fixed .message.content → .delta.content crash
  • JS: Fixed .message → .delta in streaming (3 files), replaced broken callback with for await...of

Fix documentation

  • Python test README: Fixed version 3.10+ → 3.11+
  • Rust: Fixed stale return types, removed private ModelVariant section and selected_variant() method

Copilot AI review requested due to automatic review settings April 9, 2026 05:04
@vercel
Copy link
Copy Markdown

vercel bot commented Apr 9, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
foundry-local Ready Ready Preview, Comment Apr 9, 2026 5:44am

Request Review

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates SDK validation, sample/test code, and documentation to align with the latest SDK streaming/tool-calling shapes (notably delta vs message) and to remove outdated/incorrect guidance.

Changes:

  • JS SDK: harden tool validation to avoid crashing when tool.function.description is missing.
  • Samples/tests: fix streaming chunk parsing (messagedelta) and ensure tool responses include the required ToolCallId; remove unnecessary Python async scaffolding.
  • Docs/CI: update Rust API docs to remove ModelVariant from the public surface and align return types; bump Python test README minimum version; set FOUNDRY_TESTING_MODE in test pipelines.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
sdk/rust/docs/api.md Updates Rust public API documentation to reflect the Model-only public surface and revised return types.
sdk/python/test/README.md Aligns test prerequisites with Python SDK minimum supported version.
sdk/js/src/openai/chatClient.ts Fixes tool validation crash when description is undefined.
sdk/cs/test/FoundryLocal.Tests/ChatCompletionsTests.cs Ensures tool-role messages include ToolCallId so tool-call continuations work.
samples/python/tutorial-voice-to-text/src/app.py Removes unnecessary asyncio usage in a synchronous SDK sample.
samples/python/tutorial-tool-calling/src/app.py Removes unnecessary asyncio usage and keeps sample aligned with sync SDK patterns.
samples/python/tutorial-document-summarizer/src/app.py Converts summarizer helpers/main to synchronous flow (no async/await).
samples/python/tutorial-chat-assistant/src/app.py Fixes streaming access to use delta.content and removes unnecessary async scaffolding.
samples/python/tool-calling/src/app.py Removes unnecessary asyncio usage and keeps sample aligned with sync SDK patterns.
samples/python/native-chat-completions/src/app.py Removes unnecessary asyncio usage in a synchronous SDK sample.
samples/js/tutorial-chat-assistant/app.js Updates streaming sample to for await...of and reads delta.content.
samples/js/native-chat-completions/app.js Updates streaming sample to read delta.content.
samples/js/chat-and-audio-foundry-local/src/app.js Updates streaming sample to read delta.content.
samples/cs/tool-calling-foundry-local-sdk/Program.cs Adds ToolCallId to tool-role message in the tool-calling loop.
.pipelines/templates/test-rust-steps.yml Sets FOUNDRY_TESTING_MODE for Rust unit/integration test runs.
.pipelines/templates/test-python-steps.yml Sets FOUNDRY_TESTING_MODE for Python test runs.
.pipelines/templates/test-js-steps.yml Sets FOUNDRY_TESTING_MODE for JS test runs.
.pipelines/templates/test-cs-steps.yml Sets FOUNDRY_TESTING_MODE for C# test runs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +151 to 152
| `variants` | `fn variants(&self) -> Vec<Arc<Model>>` | All variants in this model. |
| `select_variant` | `fn select_variant(&self, variant: &Model) -> Result<(), FoundryLocalError>` | Select a variant from `variants()`. |
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variants() row now returns Vec<Arc<Model>>, but the description “All variants in this model” is ambiguous because the elements are Model handles (each representing a single variant), not a ModelVariant type. Clarify that variants() returns per-variant Model instances (and for a single-variant model it returns a single-element list containing itself) to match the actual API behavior.

Suggested change
| `variants` | `fn variants(&self) -> Vec<Arc<Model>>` | All variants in this model. |
| `select_variant` | `fn select_variant(&self, variant: &Model) -> Result<(), FoundryLocalError>` | Select a variant from `variants()`. |
| `variants` | `fn variants(&self) -> Vec<Arc<Model>>` | Returns one `Model` handle per variant. For a single-variant model, this returns a single-element list containing this model itself. |
| `select_variant` | `fn select_variant(&self, variant: &Model) -> Result<(), FoundryLocalError>` | Select one of the per-variant `Model` handles returned by `variants()`. |

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May technically be correct, but could be very confusing wording

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants