-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
test: Migrate miscellaneous SDK tests to GHA Workflow #11032
Conversation
Signed-off-by: ddalvi <[email protected]>
Signed-off-by: ddalvi <[email protected]>
Signed-off-by: ddalvi <[email protected]>
run: pip install -r ./test/sdk-execution-tests/requirements.txt | ||
|
||
- name: Run component YAML tests | ||
run: ./test/presubmit-component-yaml.sh |
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'm confused. You removed kubeflow-pipelines-sdk-yapf
in https://github.com/GoogleCloudPlatform/oss-test-infra/pull/2327/files#diff-30095b445aa0ad17f1bc23514587283b5ad03959f815fb48e42bdeee2b39c066L186-L196, but in this PR you're adding a test for ./test/presubmit-component-yaml.sh
.
Was that a mistake?
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 didn't get your question. Looks like the ./test/presubmit-component-yaml.sh
runs a file: https://github.com/kubeflow/pipelines/blob/master/test/presubmit-component-yaml.sh#L27
that loads training and testing data: https://github.com/kubeflow/pipelines/blob/master/components/test_load_all_components.sh
Is this not required? If not we could take the GHA out.
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.
Let me clarify...
We have these two tests in Prow:
In this PR, you've added kubeflow-pipelines-component-yaml, but you're not removing it from Prow. Instead, you're removing kubeflow-pipelines-sdk-yapf, which you're not adding in this PR.
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.
Oops! My bad, deleted the yapf one by mistake instead of the component yaml. Updated now: https://github.com/GoogleCloudPlatform/oss-test-infra/pull/2327/files#diff-30095b445aa0ad17f1bc23514587283b5ad03959f815fb48e42bdeee2b39c066L122
Great catch!
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.
/lgtm
/hold for @rimolive's review.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hbelmiro The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/unhold |
* test: Migrate SDK isort tests to GHA Signed-off-by: ddalvi <[email protected]> * test: Migrate SDK docformatter tests to GHA Signed-off-by: ddalvi <[email protected]> * test: Migrate SDK Component YAML tests to GHA Signed-off-by: ddalvi <[email protected]> --------- Signed-off-by: ddalvi <[email protected]>
Description of your changes:
Resolves #10987
Converts the KFP SDK tests to a GH Action Workflow:
kubeflow-pipelines-sdk-isort
kubeflow-pipelines-sdk-docformatter
kubeflow-pipelines-component-yaml
Here's working GH Action runs on my fork:
kubeflow-pipelines-sdk-isort: https://github.com/DharmitD/data-science-pipelines-argo/actions/runs/10068822167/job/27834815744
kubeflow-pipelines-sdk-docformatter: https://github.com/DharmitD/data-science-pipelines-argo/actions/runs/10069220348/job/27835864296
kubeflow-pipelines-component-yaml: https://github.com/DharmitD/data-science-pipelines-argo/actions/runs/10069396931/job/27836310262
PR to remove the KFP Execution test from prow config: GoogleCloudPlatform/oss-test-infra#2327
Checklist: