[SDK] Unit tests for TrainingClient APIs - get_job_pod_names and update_job

kubeflow / training-operator

Distributed ML Training and Fine-Tuning on Kubernetes

https://www.kubeflow.org/docs/components/training

Apache License 2.0

1.61k stars 700 forks source link

[SDK] Unit tests for TrainingClient APIs - get_job_pod_names and update_job #2192

Closed YosiElias closed 2 months ago

YosiElias commented 3 months ago

What this PR does / why we need it: New unit test for get_job_pod_names and update_job functions

Which issue(s) this PR fixes (optional, in Fixes #<issue number>, #<issue number>, ... format, will close the issue(s) when PR gets merged): Fixes #2161

Checklist:

[ ] Docs included if any changes are user facing

coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 10511367867

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.009%) to 33.477%

Totals
Change from base Build 10284610088:	-0.009%
Covered Lines:	3950
Relevant Lines:	11799

💛 - Coveralls

YosiElias commented 3 months ago

can someone pls review this PR? @andreyvelich @jinchihe @kuizhiqing

andreyvelich commented 2 months ago

Hi @YosiElias! Did you get a chance to check my comment here: https://github.com/kubeflow/training-operator/pull/2192#discussion_r1722079807 ? I think after that we should be ready to merge this PR.

YosiElias commented 2 months ago

Hi @andreyvelich! Sorry for the delay, I was busy with other things. As you suggested I changed the 2 pod case (and dropped the use of SimpleNamespace) , if there are any additional comments I would love to hear them. Thank you very much for your comments!

andreyvelich commented 2 months ago

Overall, looks good, thank you for doing this @YosiElias! Please fix the pre-commit, so we can merge it. /assign @droctothorpe @kubeflow/wg-training-leads

google-oss-prow[bot] commented 2 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich

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: - ~~[sdk/python/OWNERS](https://github.com/kubeflow/training-operator/blob/master/sdk/python/OWNERS)~~ [andreyvelich] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment