Adding L1-L4 design to RFC-0050#93
Conversation
|
|
||
| #### L4: Always-On PR Checks | ||
|
|
||
| `L4` uses the same callback and synchronization behavior as `L3`, but it does not wait for a label. The `L4` scenario is effectively the same as `L3` Scenario 1 except that the downstream result is represented in upstream PR Checks by default, without requiring explicit labeling. |
There was a problem hiding this comment.
Was there a plan to allow out of tree testing to block PRs? It seems like we'd need a very tight agreement on availability if out of tree tests can halt merging on pytorch/pytorch.
There was a problem hiding this comment.
Hi @groenenboomj, we've discussed this in detail here: pytorch/pytorch#175022 (comment). For L4 level repos, they have the capabilities to gate PyTorch PR, but there will be a very high standard to achieve L4 (and the PyTorch core team remains the right to choose whether a repo is L4 even though it meets all the requirements).
There was a problem hiding this comment.
Okay, I see that was added in the last level breakdown expansion in March. Has there been more discussion on the requirements for gating?
There was a problem hiding this comment.
The main requirement is going to be this one:
Very few accelerators would reach this level. One only becomes L4 if pytorch maintainers are highly invested in it and find it worthwhile to add the signal to every PR.
It's intentionally left somewhat fuzzy, since it's a balance of "how much value will this provide vs how much pain does this inflict" since the idea itself is somewhat subjective. The automatic downgrade will need a firm reliability metric, but even that will likely need fine tuning based on experimentation, so hard numbers here in the short term are likely to be unreliable.
Do you have concerns about the approach @groenenboomj ?
There was a problem hiding this comment.
Okay, I see that was added in the last level breakdown expansion in March. Has there been more discussion on the requirements for gating?
Hey @groenenboomj, based on previous discussions with @albanD and @ZainRizvi, most accelerators will fall into L3. It’s difficult to set a well-defined standard for L4 because it is effectively equivalent to being in-tree, just with a different code location.
To me, a key requirement for L4 is broad community usage and official investment from core maintainers. Testing and feature coverage are necessary, but they aren't the only deciding factors.
There was a problem hiding this comment.
Less major concerns other than making sure we have enough formality documented so it's rigorous enough to not lead to as much debate when advancing through the levels. With that addressed it should be fine.
There was a problem hiding this comment.
Absolutely, I'm with you on that.
In my view, the CRCR consists of four levels. The formality of L1 and L2 should be clear and well-defined; I don't think many people would disagree with that. L3 isn't much of a concern either—we can progressively flesh it out and make it more concrete with input from @albanD and @ZainRizvi.
Regarding L4, I don't believe it's an urgent issue that requires immediate resolution. Since it is considered equivalent to "in-tree" accelerators like CUDA, and most accelerators won't be able to reach this level in the short term, we can afford to de-prioritize it for now.
Sure! I will discuss this with @jewelkm89 ASAP and update it to the RFC as well. |
There was a problem hiding this comment.
Hey @can-gaa-hou, the LF AI & Data Foundation is wrong, could you please correct this one in passing, it should be LF Pytorch Foundation
There was a problem hiding this comment.
Ok, let me make this change
| ``` | ||
|
|
||
| ## HUD Integration | ||
|
|
There was a problem hiding this comment.
@jewelkm89, as mentioned in https://docs.google.com/document/d/1w10Agz_YkSA1IDurobWjuNoYo4u9aykprjuymDFE6MU/edit?tab=t.5c09vnsaacwx#heading=h.t7zh9oz5xu0s, you can add the HUD section here and paste the Google Doc link for reference.
|
@fffrog Do we have signup instructions documented anywhere? E.g. If someone wants to join the L1 tier, they need to join the allowlist and add the github app to their repo. How does one find the link for the github app? |
@ZainRizvi, I added an instruction a few days ago in the PyTorch documents. Please refer to it. https://github.com/pytorch/pytorch/blob/main/docs/source/accelerator/ci.md |
Hi @ZainRizvi, Sure thing. We've added a dedicated documentation page for this. It's currently a preliminary version and will be updated as the CRCR move forward. As mentioned in the previous comments by @can-gaa-hou, the link is https://github.com/pytorch/pytorch/blob/main/docs/source/accelerator/ci.md. However, the workflow seems to be consistently failing due to timeouts. As a result, the rendered pages aren't being generated correctly, so users can currently only access the raw Markdown file rather than the properly formatted web pages. Could you please take a look at this workflow? @can-gaa-hou tried to fix it, but it seems to be beyond our scope/control |
Thanks for the pointer, will raise the problem internally. There was one bug causing that workflow to fail that was fixed already, but there seems to be another one causing the deployment step to fail as well: https://github.com/pytorch/docs/actions/workflows/pages/pages-build-deployment |
…8119) # Summary - The current implementation focuses on L3/L4 levels defined in the RFC: pytorch/rfcs#90 - For detailed design for L3/L4, please refer to pytorch/rfcs#93 # Architecture - `webhook` function: - [x] Create PR label handling function for L3 repo (refer to the 3 scenario cases mentioned in pytorch/rfcs#93) - **Scenario 1**: Label arrives before the downstream workflow job is triggered. So we cached this information in Redis using `mark_check_run_wanted` and let the `callback` lambda create this check run. - **Scenario 2**: Label arrives during the downstream workflow job is running. The `callback` lambda will cache workflow information beforehand so it can immediately create the `in_progress` check run. - **Scenario 3**: Label arrives after the downstream workflow job is done. If the cached workflow information is still alive in Redis (3 days by default, could be set by `CRCR_STATUS_TTL`), it will create a `completed` check run immediately. Otherwise, it will not create a check run. - [x] Dispatch function will check for L3 label or L4, and store this information in Redis for the `callback` to check whether a check run is needed. - [x] Create check run and check suite handling functions for the downstream workflow jobs re-run mechanism within the check run. - `callback` function: - [x] Handle upstream check-run creation/update for L3/L4 - **Scenario 1 & L4**: Check in Redis by calling `is_check_run_wanted` to see if this PR needs a check run. If so, immediately create one. - **All Scenarios**: Store workflow information in Redis for check run creation in the `webhook`. - [x] Set "in_progress" zombie check-runs to "time_out" through a sweeper periodically # Changes ```md .github/actions/cross-repo-ci-relay └── action.yml # Add a step to capture job-name for re-run aws/lambda/cross_repo_ci_relay/ ├── tests/ # Add more unit tests ├── allowlist.py # Update utils function for L3 ├── redis_helper.py # Set more keys for L3 ├── gh_helper.py # Update utils function for L3 ├── misc.py # Update utils function for L3 ├── event_handler.py # Create PR label/check run/check suites handling function ├── cleanup_handler.py # Handle zombie check-run └── callback_handler.py # Handle upstream check-run creation ``` # Verification We performed the following scenario verification on our AWS Lambda instance: - L3: - [x] L3 labels named `ciflow/crcr/{device}` are added immediately after the PR is created, and show up in the corresponding check-run on the PR with the name `crcr/{repo}/{workflow_name}/{job_name}`. - [x] After clicking into the check-run, the corresponding information is correct. - [x] L3 labels are added while the workflow job is running, which should show up the `in_progress` check-run. - [x] L3 labels are added after the workflow job is done, which should show up the `completed` check-run. - [x] Check run is updated when the PR with L3 labels is reopened or synchronized. - L4: - [x] Check run should be created after the PR is opened. - [x] Check run is updated when the PR is reopened or synchronized. - Re-run - [x] Clicking the `Re-run` button in each failed check run will trigger the corresponding downstream workflow failed jobs to re-run and update the check run status to `in_progress`. - [x] Clicking the `Re-run all jobs` or `Re-run all failed jobs` button will trigger the corresponding downstream workflow jobs in the check suite and update the corresponding check run status to `in_progress`. # Unit Tests - [x] Unit Tests (Mock) # TODO - Modify PyTorchBot for L3 non-blocking merge: pytorch/pytorch#185612 - Add a new feature in PyTorchBot that automatically mentions related on-call maintainers in L3/L4 repos when check-run fails: #8183 cc @albanD @fffrog @KarhouTam @atalman @huydhn @zxiiro @subinz1 @jewelkm89
4 participants
As @ZainRizvi mentioned here, I am opening this PR for adding L1-L4 design in our RFC. This may help us better develop L2-L4 implementation.
cc @fffrog @ZainRizvi @albanD