ci: fix the cpu test workflow to main branch#3
Conversation
ci: fix black invocation
There was a problem hiding this comment.
thanks a lot, on: pull_request_target was exactly the setting I was referring to! Do you think it would make sense to also add this to linting and documentation (not necessary for cpu_tests.yaml I think, this is anyway only for members).
There was a problem hiding this comment.
Since the other workflows are cheaper, it is fine if we do not change but I agree that it could also make sense for the other workflows.
There was a problem hiding this comment.
@sharpenb just for context - in my understanding currently the on: pull_request_target setting is not so much about when someone is allowed to run the models - it specifies that the PR is run i the context of the main branch. This means that even if someone opens a PR modifying the workflow to always run, this will not happen and only once this would be merged into the main branch (which we catch in a review).
There was a problem hiding this comment.
FYI: this prevents from updating / testing workflows from a PR, which could be a bit annoying when iterating on a new pipeline.
There was a problem hiding this comment.
After tests, this is validated :)
There was a problem hiding this comment.
Hm not sure I can 100% follow - so now we run the the tests as soon as the review is requested? I remember discussing that this tests need approval and I can't quite parse yet where this setting is in this yaml
There was a problem hiding this comment.
Yes, for first time contributors, there still is a need for an approval before being able to access the workflow.
Second time contributors won't have this gate, so they will be able to run tests directly with a review request. Is it the expected behavior, or should I change ?
There was a problem hiding this comment.
no that is perfect then, thanks!
There was a problem hiding this comment.
I think "intel" is outdated for Pruna and will no longer work
There was a problem hiding this comment.
okay this is also still present in the CPU tests, @johnrachwan123 can you confirm this is outdated?
There was a problem hiding this comment.
Let me know if I should update this :)
There was a problem hiding this comment.
yes I double checked - intel can be removed
sharpenb
left a comment
sharpenb
left a comment
There was a problem hiding this comment.
Thanks! Left a couple of comments ont he workflows. Please do not forget also to use the PR template to clearly explain the purpose of the code change :)
.github/workflows/cpu_tests.yaml
Outdated
There was a problem hiding this comment.
There is now one workflom for internal cpu tests andone for external cpu tests. Coudl we name the workflow files accordingly i.. internal/external_contribution_cpu_tests
There was a problem hiding this comment.
Since the other workflows are cheaper, it is fine if we do not change but I agree that it could also make sense for the other workflows.
SaboniAmine
force-pushed
the
ci/fix_workflows_configuration
branch
2 times, most recently
from
b51c8fd to
c53762f
Compare
SaboniAmine
force-pushed
the
ci/fix_workflows_configuration
branch
from
c53762f to
413cfbf
Compare
johannaSommer
left a comment
johannaSommer
left a comment
There was a problem hiding this comment.
LGTM Amine, thanks a lot!! We just have to remove the [intel] installation then we are good to go, approving already :)
SaboniAmine
force-pushed
the
ci/fix_workflows_configuration
branch
from
413cfbf to
1731eeb
Compare
SaboniAmine
force-pushed
the
ci/fix_workflows_configuration
branch
3 times, most recently
from
5125d18 to
1731eeb
Compare
There was a problem hiding this comment.
After tests, this is validated :)
3 participants
Description
This PR proposes a trigger configuration to fix the workflow version to the main branch.
Type of Change
Checklist