katib icon indicating copy to clipboard operation
katib copied to clipboard

[WIP] add unit tests for katib sdk

Open shashank-iitbhu opened this issue 10 months ago • 2 comments

What this PR does / why we need it:

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

Checklist:

  • [ ] Docs included if any changes are user facing

shashank-iitbhu avatar Apr 11 '24 01:04 shashank-iitbhu

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: shashank-iitbhu Once this PR has been reviewed and has the lgtm label, please assign tenzen-y for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

google-oss-prow[bot] avatar Apr 11 '24 01:04 google-oss-prow[bot]

@shashank-iitbhu Please let us know if you have time to address our comments this or next week. We are planing to cut a first Katib 0.17 RC.0 release soon.

andreyvelich avatar Apr 18 '24 10:04 andreyvelich

Hello @andreyvelich I was wondering if I can pick up on this work if you are planning on including this in Katib 0.18.

tariq-hasan avatar Apr 29 '24 06:04 tariq-hasan

Hello @andreyvelich I was wondering if I can pick up on this work if you are planning on including this in Katib 0.18.

That would be great @tariq-hasan! We can even include this in Katib 0.17 by cherry-pick this PR to the release branch. @shashank-iitbhu Are you ok to delegate this PR to @tariq-hasan so we can include that important feature to the next Katib release ?

andreyvelich avatar Apr 29 '24 12:04 andreyvelich

Hello @andreyvelich I was wondering if I can pick up on this work if you are planning on including this in Katib 0.18.

That would be great @tariq-hasan! We can even include this in Katib 0.17 by cherry-pick this PR to the release branch. @shashank-iitbhu Are you ok to delegate this PR to @tariq-hasan so we can include that important feature to the next Katib release ?

@andreyvelich I'm okay with @tariq-hasan working on this PR.

shashank-iitbhu avatar Apr 29 '24 12:04 shashank-iitbhu

Sounds good.

@andreyvelich I notice that the scope of testing in training_client_test.py is limited to create_job but that the tests were written before the train API was implemented.

I was therefore wondering if we limit the scope of katib_client_test.py to create_experiment or if we also include the tune API.

Do you plan to include tests for all the functions in training_client.py and katib_client.py in upcoming releases but limit the scope for this issue?

I plan to submit a PR within the next few days but I was also wondering if there is any strict timeline I would need to follow to get this code in the release.

tariq-hasan avatar Apr 29 '24 22:04 tariq-hasan

Sounds good.

I was therefore wondering if we limit the scope of katib_client_test.py to create_experiment or if we also include the tune API.

I think, we can start with something, it would be nice to add tests for create_experiment and tune APIs, but if you want you can add tests just for one of these APIs.

Do you plan to include tests for all the functions in training_client.py and katib_client.py in upcoming releases but limit the scope for this issue?

I don't think we have enough time to include tests for all APIs before next release.

I plan to submit a PR within the next few days but I was also wondering if there is any strict timeline I would need to follow to get this code in the release.

Sure, that sound great! If we want to include that in the Katib 0.17 release, it would be nice to merge your PR before distribution testing phase, which is May 20th: https://github.com/kubeflow/community/tree/master/releases/release-1.9

andreyvelich avatar Apr 30 '24 12:04 andreyvelich

Sounds good. I was therefore wondering if we limit the scope of katib_client_test.py to create_experiment or if we also include the tune API.

I think, we can start with something, it would be nice to add tests for create_experiment and tune APIs, but if you want you can add tests just for one of these APIs.

Do you plan to include tests for all the functions in training_client.py and katib_client.py in upcoming releases but limit the scope for this issue?

I don't think we have enough time to include tests for all APIs before next release.

I plan to submit a PR within the next few days but I was also wondering if there is any strict timeline I would need to follow to get this code in the release.

Sure, that sound great! If we want to include that in the Katib 0.17 release, it would be nice to merge your PR before distribution testing phase, which is May 20th: https://github.com/kubeflow/community/tree/master/releases/release-1.9

Sounds good. I created a PR for the create_experiment function in the katib_client module and plan to add another for the tune function once the current PR is merged.

tariq-hasan avatar May 06 '24 05:05 tariq-hasan

This was implemented here: https://github.com/kubeflow/katib/pull/2325

andreyvelich avatar Jun 20 '24 15:06 andreyvelich