Skip to content

Converted Go upgrade tests in Codeflare-SDK to python tests and add support for kueue integration #494

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 22, 2024
Merged

Converted Go upgrade tests in Codeflare-SDK to python tests and add support for kueue integration #494

merged 5 commits into from
Apr 22, 2024

Conversation

abhijeet-dhumal
Copy link
Contributor

@abhijeet-dhumal abhijeet-dhumal commented Apr 2, 2024 •
edited
Loading

Issue link

RHOAIENG-2444

What changes have been made

  • - Convert Go upgrade tests in Codeflare-SDK to python tests.
  • - Remove Go-related files since they are no longer required
  • - Update upgrade test in accordance with Kueue integration.
  • - Add support method to create kueue resources.
  • - Add PR check step to apply kueue CRD's and add cluster-roles to get/add/delete kueue resources.

Verification steps

  • Install Kueue in cluster
    oc apply --server-side -k "github.com/opendatahub-io/kueue/config/rhoai?ref=dev"
  • Create a 'Cluster-queue' resource (LocalQueue will be created in test-namespace as a part of test execution)
  • Test RayClusterSDKUpgradeTest in Openshift cluster (Execute in same order)
    poetry run pytest -v -s ./tests/upgrade/raycluster_sdk_upgrade_test.py::TestMNISTRayClusterUp
    ~ asserts RayCluster status - READY (Namespace created in TestMNISTRayClusterUp will be used in TestMnistJobSubmit )
    poetry run pytest -v -s ./tests/upgrade/raycluster_sdk_upgrade_test.py::TestMnistJobSubmit
    ~ asserts JobSubmission status

Note : In case of testing with Kind cluster, skip token authentication step.

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • Testing is not required for this change

- Manual testing followed :

image

image
image

image

ChristianZaccaria
ChristianZaccaria requested changes Apr 2, 2024
View reviewed changes
Copy link
Collaborator

@ChristianZaccaria ChristianZaccaria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work Abhijeet! Since we no longer require the use of .go files we can remove go.mod, go.sum, and the support.go file too.

Srihari1192
Srihari1192 reviewed Apr 3, 2024
View reviewed changes
Copy link
Contributor

@Srihari1192 Srihari1192 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abhijeet-dhumal Both the tests TestMNISTRayClusterUp and TestMnistJobSubmit should run independently for upgrade testing , incorporate accordingly.
you can run tests independently like this poetry run pytest -v -s ./tests/e2e/raycluster_sdk_upgrade_test.py::TestMnistJobSubmit

@abhijeet-dhumal abhijeet-dhumal marked this pull request as draft April 3, 2024 08:30
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 3, 2024
@abhijeet-dhumal abhijeet-dhumal force-pushed the python-sdk-upgrade-test branch from ce631d4 to 35b4533 Compare April 3, 2024 08:55
@abhijeet-dhumal abhijeet-dhumal marked this pull request as ready for review April 3, 2024 10:03
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 3, 2024
@abhijeet-dhumal
Copy link
Contributor Author

Great work Abhijeet! Since we no longer require the use of .go files we can remove go.mod, go.sum, and the support.go file too.

@ChristianZaccaria Thanks for reviewing, yes I have cleaned up Go files since they are no longer required

@abhijeet-dhumal
Copy link
Contributor Author

@abhijeet-dhumal Both the tests TestMNISTRayClusterUp and TestMnistJobSubmit should run independently for upgrade testing , incorporate accordingly. you can run tests independently like this poetry run pytest -v -s ./tests/e2e/raycluster_sdk_upgrade_test.py::TestMnistJobSubmit

Thank you for reviewing @Srihari1192 :)

According to above review I have made some changes wrt independent execution of tests and thoroughly tested required usecases of upgrade tests.

image

image

Srihari1192
Srihari1192 requested changes Apr 3, 2024
View reviewed changes
Copy link
Contributor

@Srihari1192 Srihari1192 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep this tests in same upgrade folder as if we run the tests with command poetry run pytest -v -s ./tests/e2e this tests also be picked and not the right way

@abhijeet-dhumal abhijeet-dhumal force-pushed the python-sdk-upgrade-test branch from 5bf9103 to 69b1e84 Compare April 3, 2024 13:17
@abhijeet-dhumal abhijeet-dhumal marked this pull request as draft April 3, 2024 13:32
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 3, 2024
@abhijeet-dhumal abhijeet-dhumal requested review from Srihari1192 and removed request for anishasthana April 3, 2024 14:57
@abhijeet-dhumal abhijeet-dhumal force-pushed the python-sdk-upgrade-test branch from 69b1e84 to b466308 Compare April 9, 2024 12:55
@abhijeet-dhumal abhijeet-dhumal changed the title Converted Go upgrade tests in Codeflare-SDK to python tests Converted Go upgrade tests in Codeflare-SDK to python tests and add support for kueue integration Apr 18, 2024
sutaakar
sutaakar approved these changes Apr 18, 2024
View reviewed changes
Copy link
Contributor

@sutaakar sutaakar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 18, 2024
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 18, 2024
@abhijeet-dhumal abhijeet-dhumal force-pushed the python-sdk-upgrade-test branch from a02de37 to 540ef4a Compare April 22, 2024 05:31
ChughShilpa
ChughShilpa approved these changes Apr 22, 2024
View reviewed changes
Copy link
Contributor

@ChughShilpa ChughShilpa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2024
@abhijeet-dhumal abhijeet-dhumal force-pushed the python-sdk-upgrade-test branch from 540ef4a to 436a7e6 Compare April 22, 2024 12:57
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2024
Bobbins228
Bobbins228 reviewed Apr 22, 2024
View reviewed changes
Copy link
Contributor

@Bobbins228 Bobbins228 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
Ran and verified it works on OpenShift

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Apr 22, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobbins228, ChughShilpa, Srihari1192, sutaakar

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2024
@sutaakar
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 4f5a930 into project-codeflare:main Apr 22, 2024
4 checks passed
openshift-merge-bot bot pushed a commit that referenced this pull request Apr 22, 2024
@abhijeet-dhumal @openshift-merge-bot
Convert SDK's Go upgrade test to Python test in accordance with kueue (…
596b5e1 
…#494)
openshift-merge-bot bot pushed a commit that referenced this pull request Apr 22, 2024
@abhijeet-dhumal @openshift-merge-bot
Adjust support methods to use exceptions and remove explicit use of m…
0cbf3a2 
…cad parameter in cluster-configuration function (#494)
openshift-merge-bot bot pushed a commit that referenced this pull request Apr 22, 2024
@abhijeet-dhumal @openshift-merge-bot
Adjust assertion of job-submission_without_login to use RayjobClient …
134e072 
…instead of REST post request (#494)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bobbins228 Bobbins228 Bobbins228 requested changes

@sutaakar sutaakar sutaakar approved these changes

@ChughShilpa ChughShilpa ChughShilpa approved these changes

@ChristianZaccaria ChristianZaccaria Awaiting requested review from ChristianZaccaria

@Srihari1192 Srihari1192 Awaiting requested review from Srihari1192

Assignees

@sutaakar sutaakar

@Srihari1192 Srihari1192

@Bobbins228 Bobbins228

@ChristianZaccaria ChristianZaccaria

@ChughShilpa ChughShilpa

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@abhijeet-dhumal @sutaakar @Srihari1192 @Bobbins228 @ChristianZaccaria @ChughShilpa