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

Adding functionality to run benchmarks for Amazon nova models #3251

Merged

Conversation

saikiranjakka
Copy link
Contributor

This changeset adds functionality to run benchmarks against Amazon nova models using the Converse API.

ref: https://docs.aws.amazon.com/bedrock/latest/userguide/conversation-inference-call.html

This changeset adds functionality to run benchmarks against Amazon nova models using the
Converse API.

ref: https://docs.aws.amazon.com/bedrock/latest/userguide/conversation-inference-call.html
@yifanmai
Copy link
Collaborator

yifanmai commented Jan 7, 2025

Looks like a duplicate of #3250; should I review this PR instead or that one?

@saikiranjakka
Copy link
Contributor Author

Yes please review the latest

Copy link
Collaborator

@yifanmai yifanmai left a comment

Choose a reason for hiding this comment

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

Looks mostly good, just a few minor requests.

src/helm/clients/bedrock_client.py Outdated Show resolved Hide resolved
src/helm/clients/bedrock_utils.py Outdated Show resolved Hide resolved
requirements.txt Outdated
@@ -23,7 +23,7 @@ awscli==1.29.85
beautifulsoup4==4.12.3
black==24.3.0
blis==1.1.0
boto3==1.28.85
boto3==1.34.131
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please also update setup.cfg here to set boto3~=1.34.131

helm/setup.cfg

Line 134 in fca12c0

boto3~=1.28.57

Copy link
Collaborator

Choose a reason for hiding this comment

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

You will need to bump the versions of awscli and botocore as well:

The conflict is caused by:
    The user requested botocore==1.31.85
    awscli 1.29.85 depends on botocore==1.31.85
    boto3 1.34.131 depends on botocore<1.35.0 and >=1.34.131

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's still some dependency conflicts. Could you revert changes to requirements.txt? That should resolve the conflict. The changes to setup.cfg should be enough. We use requirements.txt as our lockfile, and it will be automatically recomputed by our CI pipeline after you merge this PR.

src/helm/config/model_deployments.yaml Outdated Show resolved Hide resolved
@saikiranjakka
Copy link
Contributor Author

Submitted new commit with the changes

requirements.txt Outdated
@@ -23,7 +23,7 @@ awscli==1.29.85
beautifulsoup4==4.12.3
black==24.3.0
blis==1.1.0
boto3==1.28.85
boto3==1.34.131
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's still some dependency conflicts. Could you revert changes to requirements.txt? That should resolve the conflict. The changes to setup.cfg should be enough. We use requirements.txt as our lockfile, and it will be automatically recomputed by our CI pipeline after you merge this PR.

Copy link
Collaborator

@yifanmai yifanmai left a comment

Choose a reason for hiding this comment

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

Going to merge this first and resolve the dependency conflict in a new PR, for expediency.

@yifanmai yifanmai merged commit cf1f363 into stanford-crfm:main Jan 7, 2025
4 of 8 checks passed
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.

3 participants