Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bugfix] fix case for agent when memory bank registered without specifying provider_id #264

Merged
merged 3 commits into from
Oct 18, 2024

Conversation

yanxi0830
Copy link
Contributor

@yanxi0830 yanxi0830 commented Oct 17, 2024

Context

Before Fix

Calling register_memory_bank w/o provider_id fail

response = await client.register_memory_bank(
    VectorMemoryBankDef(
        identifier="test_bank2",
        embedding_model="all-MiniLM-L6-v2",
        chunk_size_in_tokens=512,
        overlap_size_in_tokens=64,
    )
)

462450019_8190267737749546_6005918137582558230_n

After Fix

image

Unit Test

image image

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 17, 2024
@yanxi0830 yanxi0830 changed the title [bugfix] fix memory bank is registered without provider_id [bugfix] fix case for agent when memory bank registered without specifying provider_id Oct 17, 2024
@yanxi0830 yanxi0830 marked this pull request as ready for review October 17, 2024 23:22
Copy link
Contributor

@ashwinb ashwinb left a comment

Choose a reason for hiding this comment

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

We could add a test case in providers/tests/agents/test_agents.py for this?

@yanxi0830
Copy link
Contributor Author

We could add a test case in providers/tests/agents/test_agents.py for this?

done!

@yanxi0830 yanxi0830 merged commit be3c5c0 into main Oct 18, 2024
4 checks passed
@yanxi0830 yanxi0830 deleted the bugfix_agent_memory branch October 18, 2024 00:28
yanxi0830 added a commit that referenced this pull request Oct 21, 2024
…fying provider_id (#264)

* fix case where memory bank is registered without provider_id

* memory test

* agents unit test
yanxi0830 added a commit that referenced this pull request Oct 21, 2024
* docker compose ollama

* comment

* update compose file

* readme for distributions

* readme

* move distribution folders

* move distribution/templates to distributions/

* rename

* kill distribution/templates

* readme

* readme

* build/developer cookbook/new api provider

* developer cookbook

* readme

* readme

* [bugfix] fix case for agent when memory bank registered without specifying provider_id (#264)

* fix case where memory bank is registered without provider_id

* memory test

* agents unit test

* Add an option to not use elastic agents for meta-reference inference (#269)

* Allow overridding checkpoint_dir via config

* Small rename

* Make all methods `async def` again; add completion() for meta-reference (#270)

PR #201 had made several changes while trying to fix issues with getting the stream=False branches of inference and agents API working. As part of this, it made a change which was slightly gratuitous. Namely, making chat_completion() and brethren "def" instead of "async def".

The rationale was that this allowed the user (within llama-stack) of this to use it as:

```
async for chunk in api.chat_completion(params)
```

However, it causes unnecessary confusion for several folks. Given that clients (e.g., llama-stack-apps) anyway use the SDK methods (which are completely isolated) this choice was not ideal. Let's revert back so the call now looks like:

```
async for chunk in await api.chat_completion(params)
```

Bonus: Added a completion() implementation for the meta-reference provider. Technically should have been another PR :)

* Improve an important error message

* update ollama for llama-guard3

* Add vLLM inference provider for OpenAI compatible vLLM server (#178)

This PR adds vLLM inference provider for OpenAI compatible vLLM server.

* Create .readthedocs.yaml

Trying out readthedocs

* Update event_logger.py (#275)

spelling error

* vllm

* build templates

* delete templates

* tmp add back build to avoid merge conflicts

* vllm

* vllm

---------

Co-authored-by: Ashwin Bharambe <[email protected]>
Co-authored-by: Ashwin Bharambe <[email protected]>
Co-authored-by: Yuan Tang <[email protected]>
Co-authored-by: raghotham <[email protected]>
Co-authored-by: nehal-a2z <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants