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

fix: Bedrock Provider request #29

Merged
merged 4 commits into from
Sep 9, 2024
Merged

fix: Bedrock Provider request #29

merged 4 commits into from
Sep 9, 2024

Conversation

andrewblane
Copy link
Collaborator

@andrewblane andrewblane commented Sep 5, 2024

Fixes Bedrock provider

  • Corrects args for request
  • Adds a 30s default timeout
    - Temporarily comments out moderator.rewrite call in generate to get Bedrock to function; need to validate that this breaks because rewrite breaks when no messages returned and handle correctly

Open questions:

  • This comments out the retry logic in exchange, assuming that it's redundant with the new retry decorator in the Provider. Do we have any reason to keep the retries in generate?

@andrewblane andrewblane changed the title Alane/fix bedrock fix: Bedrock Provider request Sep 5, 2024
@baxen
Copy link
Collaborator

baxen commented Sep 9, 2024

Open questions:

  • This comments out the retry logic in exchange, assuming that it's redundant with the new retry decorator in the Provider. Do we have any reason to keep the retries in generate?

This should be solved by #30 which we plan to merge today, we can rebase this onto it soon!

Copy link
Collaborator

@baxen baxen left a comment

Choose a reason for hiding this comment

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

LGTM! Tested with my own setup and can reproduces this fixes the integration. Let's merge after we can rebase onto the generate changes

baxen and others added 3 commits September 9, 2024 15:28
This makes the moderators compatible with providers which
don't support empty message lists
@andrewblane andrewblane marked this pull request as ready for review September 9, 2024 22:44
@andrewblane andrewblane merged commit c864235 into main Sep 9, 2024
4 checks passed
lifeizhou-ap added a commit that referenced this pull request Sep 18, 2024
* main:
  feat: Rework error handling (#48)
  chore(release): release version 0.9.0 (#45)
  chore: add just command for releases and update pyproject for changelog (#43)
  feat: convert ollama provider to an openai configuration (#34)
  fix: Bedrock Provider request (#29)
  test: Update truncate and summarize tests to check for sytem prompt t… (#42)
  chore: update test_tools to read a file instead of get a password (#38)
  fix: Use placeholder message to check tokens (#41)
  feat: rewind to user message (#30)
  chore: Update LICENSE (#40)
  fix: shouldn't hardcode truncate to gpt4o mini (#35)
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