- 
                Notifications
    You must be signed in to change notification settings 
- Fork 107
viable strict: dedup two scripts, remove checkout of test-infra #5750
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
| The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
 | 
| set -ex | ||
| output=$(python ${GITHUB_WORKSPACE}/test-infra/.github/scripts/fetch_latest_green_commit.py --requires "${{ inputs.requires }}") | ||
| TEST_INFRA_PATH="${GITHUB_ACTION_PATH}/../../.." | 
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 have an uneasy feeling about this.  My understanding is that the runner keeps all the GHA it needs locally, and because we don't have ephemeral runner, test-infra is there because some other jobs have already fetched it.  The problem here is that I'm not sure if the version of test-infra is correct, and we could end up with a stale GHA
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.
You could test this out on an ephemeral linux runner and see if test-infra is there I think
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 run I linked as the test uses runs-on: ubuntu-20.04, I assume gha runners are as ephemeral as our linux runners?  Unless maybe gha runners are more special
The version did run my the code in my branch (it errored when the argument flags were wrong)
I discovered that you don't actually have to check out the test-infra repository because it's already there?
Docs: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables#default-environment-variables
Only available for composite actions unfortunately
Testing: https://github.com/pytorch/pytorch/actions/runs/11225822033?pr=137454 (actual pushing is commented out)
There are also two fetch_latest_green_commit.py scripts in test-infra so I merged them and chose the location in tools/scripts. The logic between the two is pretty much the same. gitutils was copied from .github/scripts (but we don't need much of it)
There's also a reusable workflow update-viablestrict and a composite action update-viablestrict. The composite action is usually used, but there are some differences between the two, and vision uses the reusable workflow so I can't get rid of it just yet. (idk how much they care tho, I don't vision has succeeded updating viable/strict in a while). I also can't just make the reusable action call the composite action since the workflow uses an ssh key to check out but the action uses a token
If pytorch/vision#8676 gets merged, I'll remove the reusable workflow