updated toolkit to use Serverless Cosmos DB by default, configrable f…#6
Conversation
There was a problem hiding this comment.
Pull request overview
Updates Cosmos DB provisioning in the toolkit to default to serverless throughput behavior (no RU/s specified) while allowing an autoscale mode, and aligns change feed dependencies by provisioning the leases container alongside existing containers.
Changes:
- Add throughput mode + autoscale RU resolution helpers and wire them into sync/async Cosmos clients.
- Update
create_memory_store()to provisionmemories,counter, andleasescontainers under a shared throughput mode (serverless default; autoscale configurable). - Update Azure Functions trigger configuration, templates, docs, samples, and unit tests to reflect the new provisioning model.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
agent_memory_toolkit/_utils.py |
Adds throughput mode + autoscale RU resolution and helper for optional throughput provisioning. |
agent_memory_toolkit/cosmos_memory_client.py |
Sync client now provisions counter + lease containers and supports serverless/autoscale behavior. |
agent_memory_toolkit/aio/cosmos_memory_client.py |
Async client mirrors the sync provisioning + throughput behavior changes. |
azure_functions/function_app.py |
Disables automatic lease container creation by the trigger. |
azure_functions/local.settings.json.template |
Adds throughput mode + autoscale RU settings to local template. |
tests/unit/test_cosmos_memory_client.py |
Updates tests for lease container provisioning + serverless default + invalid throughput mode. |
tests/unit/aio/test_cosmos_memory_client.py |
Async equivalents of the updated provisioning/throughput tests. |
README.md |
Documents new constructor params and shared throughput behavior across containers. |
Docs/local_testing.md |
Updates local testing docs for serverless/autoscale provisioning and required containers. |
Docs/concepts.md |
Documents leases container provenance and shared throughput configuration. |
Docs/azure_testing.md |
Updates Azure deployment/testing guide to include throughput mode and required containers. |
Docs/README.md |
Reflects new documentation scope around throughput configuration. |
.env.template |
Adds documented env vars for throughput mode and autoscale RU cap. |
Samples/Demo.ipynb |
Updates demo to show new env vars/constructor params and provisioning notes. |
Samples/Demo_async.ipynb |
Async demo mirrors the sync demo throughput/provisioning guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aayush3011
left a comment
There was a problem hiding this comment.
Review Summary
The architecture is solid — centralizing throughput configuration into _utils.py and taking ownership of all three containers is the right move. A few issues to address before merging:
General findings (not tied to a specific diff line)
🟡 function_app.py docstring contradicts updated documentation
Every documentation file in this PR now says the leases container is "provisioned by create_memory_store()" (concepts.md, azure_testing.md, local_testing.md, README.md). However, azure_functions/function_app.py line 54 still says:
- ``leases`` – auto-created by the trigger for change feed checkpointingThis should be updated to reflect the new provisioning model, e.g.:
- ``leases`` – provisioned by create_memory_store(); trigger fallback-creates if missing🟡 No dedicated unit tests for the four new _utils.py functions
The four new functions (_resolve_cosmos_throughput_mode, _resolve_cosmos_autoscale_max_ru, _resolve_cosmos_provisioning_autoscale_max_ru, _cosmos_container_offer_throughput) are only tested indirectly through client-level tests. A dedicated tests/unit/test_utils.py would make edge-case coverage much more robust. For example, the validation bypass in the inline comment below (Finding 1) would have been caught by a simple:
def test_resolve_autoscale_max_ru_rejects_zero():
with pytest.raises(ConfigurationError):
_resolve_cosmos_autoscale_max_ru(0)
def test_resolve_autoscale_max_ru_rejects_negative():
with pytest.raises(ConfigurationError):
_resolve_cosmos_autoscale_max_ru(-1)
def test_resolve_autoscale_max_ru_rejects_bool():
with pytest.raises(ConfigurationError):
_resolve_cosmos_autoscale_max_ru(True)There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Serverless Cosmos DB by defeatul, , configrable for autoscale (1000 RU/s default, user-configurable)