ready label check

[brian-dellabetta]

SUMMARY:
To prevent merges into main where the ready label has not been applied, add a quick github actions check that errors out if it has not been applied to an open PR. This will prevent maintainers from merging into main without ready label

TEST PLAN:

• Confirmed error appears when ready label is not added. Repo owner can add this to the list of checks to prevent it from being mergeable

[gemini-code-assist]

Note

Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported.

[github-actions]

👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review.

Note: This is required to complete the testing suite, please only add the label once the PR is code complete and local testing has been performed.

[dbarbuzzi]

Looks good to me!

For clarity, this job will fail if the PR doesn't have a ready label, and will be skipped if it does.

Once the PR is merged, the repo rules/protections should be updated for the main branch to require this job (since skipped counts as passing).

[fynnsu]

Seems reasonable but would it be better if this check succeeds when labeled instead of getting skipped?

That way we could make it required to pass before the repo can be merged. That doesn't quite enforce the "transformers tests pass or get skipped" condition we want but it does at least enforce "pr is labeled ready before merge" condition.

[brian-dellabetta]

@fynnsu @dbarbuzzi thanks for the reviews. i can add another job to have it explicitly succeed if ready has been applied.

[dbarbuzzi]

That way we could make it required to pass before the repo can be merged.

The funny thing about this is, we have it set for things like the transformers check, but it appears that the reality is that it doesn't need to pass, it needs to not fail (so being skipped counts), so we can add that logic to this as-is.

I do agree that having it pass would look nicer, but this way is a tiny bit more efficient because, in the case that it is skipped (to indicate OK), there is no runner being spun up because the condition is at the job level. In order for it to pass, the condition would have to be moved, and a runner would always be required. Granted, using ubuntu-latest with such a trivial workflow should mean that it would take a couple of seconds at most from the job being triggered to complete.

So, overall, it comes down to whether we want it to look a bit nicer or have a slight efficiency edge w.r.t. skip vs. pass.

[dbarbuzzi]

The new changes won't work – in order for the job to prevent a PR via a rule, it explicitly needs to fail ("skipped" is as acceptable as "passed" with rules, it seems – at least however the transformers check rule was set up).

[rahul-tuli]

I'm fine with both skipped, or succeed cases. Thanks @brian-dellabetta

[kylesayrs]

Is there any way to make this a merge check, rather than a job failure? Personally, I'd prefer to able to look at a PR and see if it's failing any tests just by the green checkmark/red x.

[kylesayrs]

This also makes all PRs look worse and more failure-y than they might actually be

[kylesayrs]

vLLM uses the technique of keeping the longer checks in the "pending" state, and only triggering those pending checks when the tag is added

[brian-dellabetta]

vLLM uses the technique of keeping the longer checks in the "pending" state, and only triggering those pending checks when the tag is added

@kylesayrs maybe we can chat about the way vllm does it. my current changes with two separate steps is the best i've managed to get to work, but as you said will show red if the label isn't applied.