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.

Sign up for GitHub

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

Jump to bottom

feat: ✨ Implement async functionality in BedrockConverse #14326

Merged
merged 6 commits into from
Jul 19, 2024
Merged

feat: ✨ Implement async functionality in BedrockConverse #14326

merged 6 commits into from
Jul 19, 2024
+241 −26

Conversation

AndreCNF
Copy link
Contributor

@AndreCNF AndreCNF commented Jun 23, 2024 •
edited
Loading

Description

Implement async methods for the BedrockConverse LLM.

Fixes #10714
Fixes #14004

New Package?

Did I fill in the tool.llamahub section in the pyproject.toml and provide a detailed README.md for my new integration or package?

  • Yes
  • No

Version Bump?

Did I bump the version in the pyproject.toml file of the package I am updating? (Except for the llama-index-core package)

  • Yes
  • No

Type of Change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Added new unit/integration tests
  • Added new notebook (that tests end-to-end)
  • I stared at the code and made sure it makes sense

Suggested Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added Google Colab support for the newly added notebooks.
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I ran make format; make lint to appease the lint gods
@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB
@AndreCNF
Copy link
Contributor Author

AndreCNF commented Jun 23, 2024 •
edited
Loading

Currently facing an issue with aioboto3 not having converse in the bedrock-runtime service. Submitted an issue in aioboto3.
This was referenced Jun 23, 2024
Closed
Closed
@AndreCNF AndreCNF marked this pull request as ready for review July 10, 2024 14:58
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jul 10, 2024
@AndreCNF AndreCNF mentioned this pull request Jul 10, 2024
Closed
1 task
@AndreCNF
Copy link
Contributor Author

This PR is now working (check the async section of the bedrock_converse LLM example notebook) and ready for review ✅
logan-markewich
logan-markewich reviewed Jul 12, 2024
View reviewed changes
...ntegrations/llms/llama-index-llms-bedrock-converse/llama_index/llms/bedrock_converse/base.py
else:
self._client = session.client("bedrock", config=config)
self._client = session.client("bedrock", config=self._config)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious why we keep self._client but for async we use the session and create a new client each time?

I don't know which approach is better, but would be nice if there was either a good reason to diverge or if they both worked the same 👀
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I mentioned in this comment, the async client needs to be defined with an async with, otherwise I was getting an error. This error seems to be that the class wasn't being properly instantiated, as it didn't have the converse method.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you prefer to use the same method for instantiating the sync client?
Copy link
Collaborator

Choose a reason for hiding this comment

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

nah its fine, that makes sense
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jul 19, 2024
@logan-markewich logan-markewich enabled auto-merge (squash) July 19, 2024 15:14
@logan-markewich logan-markewich merged commit 7343f28 into run-llama:main Jul 19, 2024
8 checks passed
@AndreCNF AndreCNF deleted the feat/async_bedrock branch July 19, 2024 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
2 participants
@AndreCNF @logan-markewich