-
Notifications
You must be signed in to change notification settings - Fork 103
Clean up the unused sync from DynamoDB to benchmark.oss_ci_benchmark_v2 #7021
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 <[email protected]>
Signed-off-by: Huy Do <[email protected]>
@huydhn is attempting to deploy a commit to the Meta Open Source Team on Vercel. A member of the Team first needs to authorize it. |
The latest updates on your projects. Learn more about Vercel for GitHub. |
@@ -177,7 +176,7 @@ def get_doc_for_upsert(record: Any) -> Optional[Tuple[str, str, Any]]: | |||
body = handle_workflow_job(body) | |||
|
|||
id = extract_dynamodb_key(record) | |||
print(f"Parsing {id}: {json.dumps(body)}") | |||
print(f"Parsing {id}") |
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.
Could we do like a log.debug() and then not print the debug statement unless we need it?
That way we can keep the line but have it off by default.
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.
This makes sense, let me do that. The logging of this important lambda could be improved
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.
Now, I think it's better to break this into 2 PR. This will just remove the unused sync from DynamoDB to benchmark.oss_ci_benchmark_v2
. I will do the logging part separately as it's more involved.
This route is not used anymore, and always fails. I will need to clean up the write into dynamoDB in another PR and the dynamoDB table
torchci-oss-ci-benchmark
itself later.The error dropped to 0 after I removed this line:
Also remove the printI'll do this in a separate PR to improve the logging mechanism here instead of using poor manjson.dumps(body)
part because it's writing too much information into the log. We needed it at the start to debug issue, but it's too verbose now. We can remove the whole print statement now if no one really looks into it. @clee2000 Your thoughts on this?print