Stop re-implementing the log upload in backfill_events.py - #8596
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
meta-cla
Bot
added
the
CLA Signed
| def upload_log(owner: str, repo: str, job_id: int, conclusion: str) -> None: | ||
| if not LOG_UPLOADER_BOT_KEY: | ||
| warn("LOG_UPLOADER_BOT_KEY is not set, skipping the log upload...") | ||
| return |
There was a problem hiding this comment.
let's make failure explicit? or validate the key presence early
🟡 Without `LOG_UPLOADER_BOT_KEY` the script skips every log upload without failing. (ai-generated section)
upload_log warns and returns for every job when the key is empty, and it is called once per job from process_workflow_run. Under Python's default warning filter that identical warning is emitted only once per process, so a run over thousands of jobs prints it a single time, early, among the repeated ..Processing N jobs... output, then keeps writing DynamoDB rows; an otherwise successful run exits 0 with no log uploaded. A key that is set but wrong is louder — the route answers 403 and the warning carries the job id, so it repeats per job — but that run also ends successfully with nothing uploaded. Validating the key once in backfill() before the loop starts, and stopping on the first 401 or 403, puts both decisions where the operator can still act on them.
Reviewed by codex gpt-5.6-sol at xhigh effort, against 5a2abf5.
| + f"{error}, skipping..." | ||
| f"Failed to request a log upload for job {job_id} from repo " | ||
| f"{owner}/{repo}: {error}, skipping..." | ||
| ) |
There was a problem hiding this comment.
comment consistency nit.
⚪ The comment above the `try` block describes a failure this block cannot see. (ai-generated section)
A 200 from /api/log-uploader/backfill means the upload was queued for an asynchronous Lambda invocation, not that GitHub still had the log — so an expired log cannot produce a warning at this call site, and the download error appears only in the uploader lambda's own logs. Please drop the expiration comment or replace it with what this block does handle, which is the POST and its network failures.
Reviewed by codex gpt-5.6-sol at xhigh effort, against 5a2abf5.
2 participants
Stack from ghstack (oldest at bottom):
Impact:
tools/scripts/backfill_events.py, a manually run scriptRisk: low
What
upload_lognow POSTs to torchci's/api/log-uploader/backfillroute instead ofdownloading the log, gzipping it, putting it in S3, and pinging the classifier
itself. Needs
LOG_UPLOADER_BOT_KEY, and honoursHUD_URLfor testing against apreview deployment.
Also drops the S3 raw-event archive write from
process_event, leaving only theDynamoDB write.