FIX Bedrock LM produces extraneous key error in unclear circumstances

[aazizisoufiane]

Bedrock LM produces extraneous key error in unclear circumstances #365

[JamesScharf]

Bedrock seems to be moving towards a new API (the "messages" API) that mirrors the OpenAI spec. Maybe it makes sense to also remove the aws.py file and change Bedrock to BedrockCompletionsLLM? It should work for Claude 2 and Mistral models on Bedrock, but not Claude 3.

[arnavsinghvi11]

Hi @aazizisoufiane @JamesScharf , does #720 handle this? wondering if we can close this PR and make the changes to Bedrock as @JamesScharf suggests?

[soufianeaazizi]

No, we still need :
if "n" in kwargs.keys():
del kwargs["n"]
in AWSLM when using anthropic.claude-instant-v1
If you can add it in your PR, then we can close this one

[aazizisoufiane]

@arnavsinghvi11 @JamesScharf #720 with the new PR, I got also this error with bedrock botocore.errorfactory.ValidationException: An error occurred (ValidationException) when calling the InvokeModel operation: max_tokens_to_sample: range: 1..1,000,000

[mikeusru]

It seems like an edit to the super().init call in the Bedrock class still needs to be changed - batch_n should be False
batch_n=False

[arnavsinghvi11]

@drawal1 it seems like we could incorporate some of the AWS model error handling (particularly backoff) from this PR. feel free to suggest any comments on how to merge with the current integrations.

[drawal1]

@arnavsinghvi11 @aazizisoufiane Both the files in this PR are now obsolete.

Based on a review of the backoff handler, the changes belong in the aws_providers.py file. Specifically, the call_model functions in the Bedrock and Sagemaker classes in aws_providers.py should be decorated with:

    @backoff.on_exception(
        backoff.expo,
        ERRORS,
        max_time=1000,
        max_tries=8,
        on_backoff=backoff_hdlr,
        giveup=giveup_hdlr,
    )

The backoff code should also reside in this file

[drawal1]

It seems like an edit to the super().init call in the Bedrock class still needs to be changed - batch_n should be False batch_n=False

@mikeusru - I'm not so sure about this. Pretty sure batching is now implemented in Bedrock although not all models may be supported

[arnavsinghvi11]

@arnavsinghvi11 @aazizisoufiane Both the files in this PR are now obsolete.

Based on a review of the backoff handler, the changes belong in the aws_providers.py file. Specifically, the call_model functions in the Bedrock and Sagemaker classes in aws_providers.py should be decorated with:

    @backoff.on_exception(
        backoff.expo,
        ERRORS,
        max_time=1000,
        max_tries=8,
        on_backoff=backoff_hdlr,
        giveup=giveup_hdlr,
    )

The backoff code should also reside in this file

got it. @aazizisoufiane could you refactor this PR to work with aws_providers.py? Thanks!

[aazizisoufiane]

@arnavsinghvi11 , thanks, the refactoring is done

[drawal1]

@aazizisoufiane Can you verify no regressions with tests/modules/test_aws_models.py?

[aazizisoufiane]

@drawal1, I have successfully run tests for the Anthropic model. However, I'm unable to test the other models as I don't have the necessary access rights on AWS. Nevertheless, there is no reason to believe they would not pass.

[arnavsinghvi11]

Hi @aazizisoufiane , can you run ruff check . --fix-only and push again? should be good to merge after this!

[aazizisoufiane]

@arnavsinghvi11 done

[arnavsinghvi11]

Thanks @aazizisoufiane !