-
Notifications
You must be signed in to change notification settings - Fork 100
Add a lambda to write benchmark results #6718
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
Conversation
Signed-off-by: Huy Do <huydhn@gmail.com>
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
Signed-off-by: Huy Do <huydhn@gmail.com>
Signed-off-by: Huy Do <huydhn@gmail.com>
Signed-off-by: Huy Do <huydhn@gmail.com>
Signed-off-by: Huy Do <huydhn@gmail.com>
Signed-off-by: Huy Do <huydhn@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. My comments are just opinions so feel free to do with them what you think is best.
), | ||
} | ||
|
||
# Upload the content to S3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this comment is unnecessary since the function name it's commenting on is already descriptive.
"body": json.dumps({"message": f"Missing required parameter: {str(e)}"}), | ||
} | ||
|
||
# Check if the path already exists in the bucket |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this comment is unnecessary since check_path_exists() is already self describing function name.
), | ||
} | ||
|
||
# Extract authentication parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code below imo is pretty clear. I don't think the comment here is necessary.
"body": json.dumps({"message": "Authentication credentials are required"}), | ||
} | ||
|
||
# Validate authentication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code below imo is pretty clear. I don't think the comment here is necessary.
"body": json.dumps({"message": "Invalid authentication credentials"}), | ||
} | ||
|
||
# Extract input parameters from the event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in this case the comment gives less detail the the code it's describing. I'd just remove the comment altogether.
Let me clear up those redundant comments, I guess Claude wants to make sure that I understand what it is doing :) |
Signed-off-by: Huy Do <huydhn@gmail.com>
Courtesy of Claude code. This is the initial version of an API that I'm building to allow people to upload benchmark results. This works in conjunction with pytorch/pytorch-integration-testing#36
I will need to prepare a Terraform change for the lambda, but the API works as follows:
cc @zhe-thoughts @ZainRizvi