Add unit tests to the Katib SDK

[droctothorpe]

/kind feature

Describe the solution you'd like
The Katib SDK should have unit tests.

Anything else you would like to add:
We'd be happy to contribute them.

---

Love this feature? Give it a 👍 We prioritize the features with the most 👍

[andreyvelich]

That would be awesome if you could help us with the SDK unit tests @droctothorpe!

Do you want to add unit test for KatibClient or for Katib SDK models as well ?
Previously, we disabled generation of test files for models since it was broken: https://github.com/kubeflow/katib/blob/master/hack/gen-python-sdk/post_gen.py#L91-L101

[droctothorpe]

We'd probably start with the various KatibClient methods and branch out from there. We'd probably roll our own test files instead of generating them since we want to test the SDK logic as opposed to the generated client code. Do you mind storing test files alongside the modules they're testing? That's what KFP does.

[andreyvelich]

Sure, thanks @droctothorpe!
Do you mean creating sdk/python/v1beta1/kubeflow/katib/api/katib_client_test.py for unit tests ?
I guess, even Python unittest recommends it: https://docs.python.org/3/library/unittest.html#test-discovery.
In that case, we might want to refactor our tests for Suggestions:
https://github.com/kubeflow/katib/tree/master/test/unit/v1beta1/suggestion to locate them near original files.
WDYT @tenzen-y @johnugeorge @gaocegege @droctothorpe ?

[droctothorpe]

FWIW, I prefer storing unit test files alongside the modules they're testing because it makes iterating on them easier since they're side by side, but I've seen it both ways and am not strongly opinionated one way or the other.

Do ya'll have a preference between unittest and pytest? FWIW, the KFP SDK is migrating to pytest. Long term, it would be awesome if the various Kubeflow SDKs (KFP, katib, training operator) standardized and consolidated in a way that promoted reuse.

[tenzen-y]

https://github.com/kubeflow/katib/tree/master/test/unit/v1beta1/suggestion to locate them near original files.
WDYT @tenzen-y @johnugeorge @gaocegege @droctothorpe ?

I don't have a strong opinion. However, we should select either way, not adopt both ways.

[tenzen-y]

Do ya'll have a preference between unittest and pytest? FWIW, the KFP SDK is migrating to pytest. Long term, it would be awesome if the various Kubeflow SDKs (KFP, katib, training operator) standardized and consolidated in a way that promoted reuse.

It's a good point. Currently, although the katib uses both libs (pytest and unittest), we should select either way.
So, +1 on pytest to standardize the whole of kubeflow SDKs in a way.

[andreyvelich]

Yes, we should use pytest. I guess, we are already using it for our Suggestion unit test:

katib/test/unit/v1beta1/suggestion/test_optuna_service.py

Line 17 in e69235d

import pytest

I am also ok with storing test files alongside the actual files, similar to KFP.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[andreyvelich]

/lifecycle frozen

[shashank-iitbhu]

Hey, IMO, it would be great to store test files alongside the actual files and standardize testing across all Kubeflow SDKs.
I can start by going through the methods defined in katib_client.py and create corresponding test cases by mocking them. For example, in the create_experiment method, we can create a mock experiment using the V1beta1Experiment class and then also simulate the TimeoutError and RuntimeError. And later move on to katib/models for writing unit tests.

I would like to work on this issue. Please assign @andreyvelich .

[andreyvelich]

That would be great, thank you @shashank-iitbhu!
That's right, we already added test for TrainingClient in Training Operator SDK using pytest
We should follow the same logic for Katib Client.

I will assign this issue to you. Feel free to ask me any questions.
/assign @shashank-iitbhu

[andreyvelich]

Hi @shashank-iitbhu, did you have time to work on that issue ?
We want to include this feature in the next Kubeflow release and the feature freeze date is next week.

[shashank-iitbhu]

Hi @shashank-iitbhu, did you have time to work on that issue ? We want to include this feature in the next Kubeflow release and the feature freeze date is next week.

Hey @andreyvelich, I've opened a pull request #2305 for the unit tests. I aim to complete this by the weekend, before the feature freeze date. My apologies for the delayed response, I got caught up with other commitments. Additionally, navigating a new project is a bit overwhelming for me.