-
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 KFP SDK Tests to GHA #11037
Conversation
73d131a
to
9f0aaa3
Compare
created an issue to track the one failing SDK test: #11038 |
.github/workflows/kfp-sdk-tests.yml
Outdated
pull_request: | ||
paths: | ||
- 'sdk/**' | ||
- 'test/presubmit-tests-sdk.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.
test/presubmit-tests-sdk.sh
is no longer used.
We can remove this line and also the file.
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.
Removed this line and file.
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 still see this line. The file was removed though.
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.
Not sure how this change didn't get pushed, but pushed it again, should be corrected now.
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.
@hbelmiro shall I make a separate PR for removal of test/presubmit-tests-sdk.sh
file? Removing the file causes Prow runs to fail and that blocks merging of 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.
Added the script back based on Diego's suggestion here: #11037 (comment)
sdk/python/kfp/cli/component_test.py
Outdated
|
||
# Commenting this test out as it's failing. |
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.
Instead of commenting out the test, can we use @unittest.skip
? This approach preserves the code, helping us avoid breaking it and maintaining easy access to references.
More info about @unittest.skip
: https://docs.python.org/3/library/unittest.html#skipping-tests-and-expected-failures
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.
Can you also add a comment with a link to #11038?
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.
Done, added @unittest.skip
statement instead of commenting out the test, and added a comment with a link to #11038
.github/workflows/kfp-sdk-tests.yml
Outdated
paths: | ||
- 'sdk/**' | ||
- 'test/presubmit-tests-sdk.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 think this can be removed.
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.
Done, removed this.
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 still see it.
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.
Pushed again, changes should be visible now.
.github/workflows/kfp-sdk-tests.yml
Outdated
paths: | ||
- 'sdk/**' | ||
- 'test/presubmit-tests-sdk.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.
We should also add .github/workflows/kfp-sdk-tests.yml
here.
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.
Done, added.
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 don't see this change.
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.
Pushed again, changes should be visible now.
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.
@DharmitD thank you for addressing my suggestions.
But it seems like you forgot to push some commits or something.
.github/workflows/kfp-sdk-tests.yml
Outdated
paths: | ||
- 'sdk/**' | ||
- 'test/presubmit-tests-sdk.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 still see it.
.github/workflows/kfp-sdk-tests.yml
Outdated
pull_request: | ||
paths: | ||
- 'sdk/**' | ||
- 'test/presubmit-tests-sdk.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 still see this line. The file was removed though.
.github/workflows/kfp-sdk-tests.yml
Outdated
paths: | ||
- 'sdk/**' | ||
- 'test/presubmit-tests-sdk.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 don't see this change.
python-version: ${{ matrix.python }} | ||
|
||
- name: Install dependencies | ||
run: | |
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.
May you keep the bash script? In this case, we can call it locally in order to have it working.
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.
Done, I added the script back.
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.
Yes. Now, delegate the yaml to that script. In this case, locally and GHA will be on sync
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.
Done, updated to use the script here.
b0b61a2
to
1a974c9
Compare
Signed-off-by: ddalvi <[email protected]>
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
/lgtm |
/lgtm |
@chensun could you please approve? Thanks. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chensun, 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 |
Signed-off-by: ddalvi <[email protected]>
Description of your changes:
Resolves #10987
Converts the KFP SDK tests to a GH Action Workflow.
Here's working GH Action run on my fork: https://github.com/DharmitD/data-science-pipelines-argo/actions/runs/10080983836
PR to remove the KFP Execution test from prow config: GoogleCloudPlatform/oss-test-infra#2329
Checklist: