Skip to content

Conversation

mengfei25
Copy link
Contributor

@mengfei25 mengfei25 commented Jun 24, 2025

disable_all

@mengfei25 mengfei25 requested a review from chuanqi129 June 25, 2025 01:04
@mengfei25
Copy link
Contributor Author

mengfei25 commented Jun 25, 2025

@mengfei25 mengfei25 changed the title [CI] Set read-all for action workflows [CI] Set read-all for action workflows top-level token-permissions Jun 25, 2025
@mengfei25 mengfei25 changed the title [CI] Set read-all for action workflows top-level token-permissions [CI] Set read-all for workflows top-level token-permissions Jun 25, 2025
@mengfei25 mengfei25 added this pull request to the merge queue Jun 26, 2025
Merged via the queue into main with commit d9b81b8 Jun 26, 2025
12 checks passed
@mengfei25 mengfei25 deleted the mengfeil/modify-permission branch June 26, 2025 01:58
dvrogozh added a commit to dvrogozh/torch-xpu-ops that referenced this pull request Jul 7, 2025
This commit reverts some changes introduced in d9b81b8. The changes:
* Introduce some undocumented mechanism to enable/disable CI checks
* Did not account that workflow might be triggered per schedule

The changes can be reintroduced if 1) documented, 2) work properly.

Fixes: d9b81b8 ("[CI] Set read-all for workflows top-level token-permissions (intel#1777)")
Signed-off-by: Dmitry Rogozhkin <[email protected]>
@dvrogozh
Copy link
Contributor

dvrogozh commented Jul 7, 2025

@mengfei25 : this PR breaks accelerate and transformers tests scheduled to execute weekly:

+ gh --repo intel/torch-xpu-ops pr view
+ tee pr-info.txt
argument required when using the --repo flag

Usage:  gh pr view [<number> | <url> | <branch>] [flags]

Flags:
  -c, --comments          View pull request comments
  -q, --jq expression     Filter JSON output using a jq expression
      --json fields       Output JSON with the specified fields
  -t, --template string   Format JSON output using a Go template; see "gh help formatting"
  -w, --web               Open a pull request in the browser
  
Error: Process completed with exit code 1.

The change you've introduced adds some mechanism to skip these tests, right? It does so based on commit/PR description? Unfortunately there is no any description provided to understand how it works. As such it's useless as can be used by limited number of people who knows how it works. Secondly, as I noted, it effectively breaks CI as it does not account on the scheduled workflows and handles only PRs.

Further, the 1777 PR title says to add read-all permissions. Why does it adjust any workflows to add enable/disable mechanism? That's the change unrelated to the subject. Even if some workflows are long/failing/etc. for permissions unrelated reasons - we should not go and start enabling/disabling them.

I think the right strategy in your case was to request an exception to merge the change on some failing cases in Accelerate/Transformers. You also could break that into 2 steps: modify all workflows except Accelerate/Transformers and then continue working (with me for example) to change the remaining 2.

Overall, I strongly suggest that we need to revert some of the changes from PR 1777 for accelerate and transformers. Once done, you can reintroduce the fixed and described changes. Please, review:

@mengfei25
Copy link
Contributor Author

@mengfei25 : this PR breaks accelerate and transformers tests scheduled to execute weekly:

+ gh --repo intel/torch-xpu-ops pr view
+ tee pr-info.txt
argument required when using the --repo flag

Usage:  gh pr view [<number> | <url> | <branch>] [flags]

Flags:
  -c, --comments          View pull request comments
  -q, --jq expression     Filter JSON output using a jq expression
      --json fields       Output JSON with the specified fields
  -t, --template string   Format JSON output using a Go template; see "gh help formatting"
  -w, --web               Open a pull request in the browser
  
Error: Process completed with exit code 1.

The change you've introduced adds some mechanism to skip these tests, right? It does so based on commit/PR description? Unfortunately there is no any description provided to understand how it works. As such it's useless as can be used by limited number of people who knows how it works. Secondly, as I noted, it effectively breaks CI as it does not account on the scheduled workflows and handles only PRs.

Further, the 1777 PR title says to add read-all permissions. Why does it adjust any workflows to add enable/disable mechanism? That's the change unrelated to the subject. Even if some workflows are long/failing/etc. for permissions unrelated reasons - we should not go and start enabling/disabling them.

I think the right strategy in your case was to request an exception to merge the change on some failing cases in Accelerate/Transformers. You also could break that into 2 steps: modify all workflows except Accelerate/Transformers and then continue working (with me for example) to change the remaining 2.

Overall, I strongly suggest that we need to revert some of the changes from PR 1777 for accelerate and transformers. Once done, you can reintroduce the fixed and described changes. Please, review:

Fixed in #1846
https://github.com/intel/torch-xpu-ops/pull/1846/files#diff-8fa2c037d3633c466cf7c27fff1d25ff32b8993a024a3b359166d720b2da815e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants