Update

[ghstack-poisoned]

Author: huydhn

aws/lambda/gha-log-uploader/README.md

@@ -33,31 +33,27 @@ twice and then DLQs it.
 
 ## Classification
 
-After a log is stored, `log_classifier` is invoked with `InvocationType: "Event"`
-and the result is not awaited. `github-status-test` called it with an untimed
-`urlopen` and blocked until classification finished, which is what produced its
-274s/344s/400s/900s duration tails — the fix is the invocation type, not a
-separate function.
-
-It is reached through `lambda:InvokeFunction` rather than its public function URL
-(`AuthType: NONE`), so the path from here to classification never crosses a
-public endpoint. A function URL would not work anyway: those only support the
-`RequestResponse` invocation type, so calling one means waiting for
-classification to finish.
-
-`log_classifier` builds on `lambda_http` with only the `apigw_http` feature, so
-`classifier_payload()` reproduces the API Gateway HTTP API v2.0 request it
-expects even on a direct invoke. Verified against the deployed function: a
-payload with no `job_id` returns its 400 "no job id provided" branch, and a
-non-numeric one fails inside its `parse::<usize>()`, which together show both the
-envelope and the query string are read.
-
-That envelope is coupling to another lambda's framework, and it would break if
-`log_classifier`'s handler changed. Teaching it to accept a plain
-`{"job_id", "repo"}` payload is the real fix, and lets its public function URL be
-retired once `backfillJobs.mjs`, `keep-going-call-log-classifier` and
-`github-status-test` move off it too — worth doing on its own, not as a rider on
-this migration.
+After a log is stored, `log_classifier` is called through its function URL —
+byte for byte the call `github-status-test` makes today.
+
+That call is synchronous. Function URLs only support the `RequestResponse`
+invocation type, so this function's duration includes the classification, and
+`github-status-test`'s 274s/344s/400s/900s duration maxima come from exactly
+this. **Keep the timeout at 900s**: on a slow classification a shorter one would
+kill the invocation mid-wait, and since callers invoke asynchronously, Lambda
+would then retry the whole thing and re-download the log.
+
+Unlike in `github-status-test` the tail is no longer harmful. There it ran behind
+API Gateway on the webhook's critical path, so a slow classification risked a
+GitHub webhook timeout. Here the caller has already returned, and a long
+invocation costs GB-seconds and a concurrency slot, nothing more.
+
+The way out is `lambda:InvokeFunction` with `InvocationType: "Event"`, which
+needs `log_classifier` to accept a plain `{"job_id", "repo"}` payload — it
+currently only parses the API Gateway request its `lambda_http` handler expects.
+That change also lets its `AuthType: NONE` function URL be retired, once
+`backfillJobs.mjs`, `keep-going-call-log-classifier` and `github-status-test`
+move off it. Worth doing on its own, not as a rider on this migration.
 
 A failed handoff is logged and reported as `classified: false`, not raised.
 Raising would make Lambda retry the whole function, re-downloading a
@@ -112,13 +108,12 @@ Notes on the app path:
 
 Not done by CI. Needed before the deploy workflow can run.
 
-1. Create the function: python3.12, x86_64, handler `lambda_function.lambda_handler`.
-   512 MB and a 60s timeout are plenty — the old function averaged 200ms and its
-   long tail was the classifier ping this one does not make.
-2. Give its execution role `s3:PutObject` on `arn:aws:s3:::ossci-raw-job-status/log/*`,
-   `lambda:InvokeFunction` on
-   `arn:aws:lambda:us-east-1:308535385114:function:log_classifier`, plus the
-   usual CloudWatch Logs permissions.
+1. Create the function: python3.12, x86_64, handler `lambda_function.lambda_handler`,
+   512 MB, **900s timeout** — matching `github-status-test`, because the
+   synchronous classifier call means a slow classification is a slow invocation.
+2. Give its execution role `s3:PutObject` on `arn:aws:s3:::ossci-raw-job-status/log/*`
+   plus the usual CloudWatch Logs permissions. No `lambda:InvokeFunction` is
+   needed while the classifier is reached over its function URL.
 3. Set the env vars above. Prefer fresh credentials over copying
    `github-status-test`'s, whose PATs sit in plaintext env vars and are due for
    rotation.
@@ -134,9 +129,8 @@ Not done by CI. Needed before the deploy workflow can run.
    `OUR_AWS_ACCESS_KEY_ID` before granting.
 6. Create the `gha_workflow_gha-log-uploader-lambda` IAM role the deploy workflow
    assumes, mirroring `gha_workflow_github-status-test-lambda`.
-7. Nothing to wire for classification: this function invokes `log_classifier`
-   directly, so there is no S3 notification to add. `keep-going-call-log-classifier`
-   still covers the separate `temp_logs/` prefix on `gha-artifacts`.
+7. Nothing to wire for classification: the classifier is called over its existing
+   function URL, so there is no notification or extra permission to add.
 
 ## Deployment
 

aws/lambda/gha-log-uploader/lambda_function.py

@@ -7,19 +7,20 @@
 API Gateway integration and no Lambda function URL: the only way in is
 ``lambda:InvokeFunction``, which is IAM-authenticated.
 
-The classifier is invoked the same way, asynchronously, so this function never
-blocks on classification finishing. ``github-status-test`` called it with an
-untimed ``urlopen`` and waited, which is what produced its 274s/344s/400s/900s
-duration tails -- the fix is the invocation type, not a separate function.
+Classification is kicked off exactly as ``github-status-test`` does it, through
+log_classifier's function URL. That call is synchronous, so this function's
+duration includes the classification -- which is why its timeout has to stay at
+900s. Switching to an async ``lambda:InvokeFunction`` needs log_classifier to
+accept a plain payload first; see the README.
 """
 
 import base64
 import contextlib
 import gzip
-import json
 import os
 import random
 import time
+from urllib.request import urlopen
 
 import boto3
 import requests
@@ -28,13 +29,12 @@
 
 
 s3 = boto3.resource("s3")
-lambda_client = boto3.client("lambda")
 GITHUB_TOKENS = os.environ.get("GITHUB_TOKENS")
 GITHUB_APP_ID = os.environ.get("GITHUB_APP_ID")
 # Base64-encoded PEM, the same encoding torchci uses for its app key
 GITHUB_APP_PRIVATE_KEY = os.environ.get("GITHUB_APP_PRIVATE_KEY")
 BUCKET_NAME = "ossci-raw-job-status"
-LOG_CLASSIFIER_FUNCTION = "log_classifier"
+LOG_CLASSIFIER_URL = "https://vwg52br27lx5oymv4ouejwf4re0akoeg.lambda-url.us-east-1.on.aws"
 
 GITHUB_API_URL = "https://api.github.com"
 # Installation tokens last an hour. Refresh early so a warm invocation never
@@ -135,66 +135,21 @@ def log_object_path(full_name, job_id):
     return f"log/{full_name}/{job_id}"
 
 
-def classifier_payload(full_name, job_id):
-    """An API Gateway HTTP API v2.0 request, which is what lambda_http parses.
-
-    log_classifier is built on lambda_http with only the `apigw_http` feature, so
-    it expects this envelope even on a direct invoke. Verified against the
-    deployed function: a payload with no `job_id` returns its 400 "no job id
-    provided" branch, and a non-numeric one fails in its `parse::<usize>()`,
-    which together show both the envelope and the query string are read.
-    """
-    return {
-        "version": "2.0",
-        "routeKey": "$default",
-        "rawPath": "/",
-        "rawQueryString": f"job_id={job_id}&repo={full_name}",
-        "headers": {},
-        "queryStringParameters": {"job_id": str(job_id), "repo": full_name},
-        "requestContext": {
-            "accountId": "308535385114",
-            "apiId": "gha-log-uploader",
-            "domainName": "lambda-invoke",
-            "domainPrefix": "lambda-invoke",
-            "http": {
-                "method": "GET",
-                "path": "/",
-                "protocol": "HTTP/1.1",
-                "sourceIp": "127.0.0.1",
-                "userAgent": "gha-log-uploader",
-            },
-            "requestId": f"gha-log-uploader-{job_id}",
-            "routeKey": "$default",
-            "stage": "$default",
-            "time": "01/Jan/1970:00:00:00 +0000",
-            "timeEpoch": 0,
-        },
-        "isBase64Encoded": False,
-    }
-
-
 def classify_log(full_name, job_id):
-    """Kick off classification for a log we just stored. Returns True on handoff.
-
-    Asynchronous, and reached through `lambda:InvokeFunction` rather than
-    log_classifier's public function URL, so the path from here to classification
-    never crosses a public endpoint. A function URL would not do: it only supports
-    the RequestResponse invocation type, so using one means waiting for
-    classification to finish -- which is exactly the mistake that gave
-    github-status-test its multi-hundred-second tails.
+    """Kick off classification for a log we just stored. Returns True on success.
+
+    Same call github-status-test makes. The function URL only supports the
+    RequestResponse invocation type, so this blocks until classification
+    finishes; the function's 900s timeout has to cover that.
     """
     try:
-        lambda_client.invoke(
-            FunctionName=LOG_CLASSIFIER_FUNCTION,
-            InvocationType="Event",
-            Payload=json.dumps(classifier_payload(full_name, job_id)).encode(),
-        )
+        urlopen(f"{LOG_CLASSIFIER_URL}/?job_id={job_id}&repo={full_name}")
         return True
     except Exception as err:
         # Best effort, deliberately. Raising would make Lambda retry the whole
-        # function, re-downloading a multi-megabyte log from GitHub to retry a
-        # handoff that takes milliseconds. The log itself is already safe in S3.
-        print(f"ERROR invoking the classifier for {full_name} job {job_id}: {err}")
+        # function and re-download a multi-megabyte log from GitHub, when the log
+        # itself is already safe in S3.
+        print(f"ERROR calling the classifier for {full_name} job {job_id}: {err}")
         return False
 
 

aws/lambda/gha-log-uploader/test_lambda_function.py

@@ -1,10 +1,8 @@
-import json
 import unittest
 from unittest.mock import MagicMock, patch
 
 import lambda_function
 from lambda_function import (
-    classifier_payload,
     classify_log,
     download_log,
     installation_token,
@@ -56,43 +54,20 @@ def test_rejects_a_non_object_payload(self):
             parse_event(["pytorch/pytorch", 123])
 
 
-class TestClassifierPayload(unittest.TestCase):
-    def test_is_a_v2_request_the_classifier_can_parse(self):
-        payload = classifier_payload("pytorch/executorch", 999)
-        # log_classifier builds on lambda_http with only the apigw_http feature,
-        # so version 2.0 and requestContext.http are what make it deserialize.
-        self.assertEqual(payload["version"], "2.0")
-        self.assertIn("http", payload["requestContext"])
-        self.assertEqual(
-            payload["queryStringParameters"],
-            {"job_id": "999", "repo": "pytorch/executorch"},
-        )
-        self.assertEqual(payload["rawQueryString"], "job_id=999&repo=pytorch/executorch")
-
-    def test_is_json_serializable(self):
-        json.dumps(classifier_payload("pytorch/pytorch", 1))
-
-
 class TestClassifyLog(unittest.TestCase):
-    def test_invokes_the_classifier_asynchronously(self):
-        with patch.object(lambda_function, "lambda_client") as client:
+    def test_calls_the_classifier(self):
+        with patch.object(lambda_function, "urlopen") as urlopen:
             self.assertTrue(classify_log("pytorch/pytorch", 123))
 
-        kwargs = client.invoke.call_args.kwargs
-        self.assertEqual(kwargs["FunctionName"], "log_classifier")
-        # Event, not RequestResponse: waiting on classification is exactly the
-        # mistake that gave github-status-test its multi-hundred-second tails.
-        self.assertEqual(kwargs["InvocationType"], "Event")
-        self.assertEqual(
-            json.loads(kwargs["Payload"])["queryStringParameters"],
-            {"job_id": "123", "repo": "pytorch/pytorch"},
+        urlopen.assert_called_once_with(
+            f"{lambda_function.LOG_CLASSIFIER_URL}/?job_id=123&repo=pytorch/pytorch"
         )
 
-    def test_a_failed_invoke_is_reported_not_raised(self):
-        # Raising would make Lambda retry the whole function, re-downloading a
-        # multi-megabyte log to retry a handoff that takes milliseconds.
-        with patch.object(lambda_function, "lambda_client") as client:
-            client.invoke.side_effect = RuntimeError("throttled")
+    def test_a_failed_call_is_reported_not_raised(self):
+        # Raising would make Lambda retry the whole function and re-download a
+        # multi-megabyte log, when the log is already safe in S3.
+        with patch.object(lambda_function, "urlopen") as urlopen:
+            urlopen.side_effect = RuntimeError("connection reset")
             self.assertFalse(classify_log("pytorch/pytorch", 123))