Add unit test for `create_experiment` in the `katib_client` module #2325

tariq-hasan · 2024-05-06T05:39:05Z

What this PR does / why we need it:

Unit test has been added for the create_experiment function as part of this commit.
Code refactoring has been done as part of other commits in the PR.
I plan to create a second PR to add unit test for the tune function once this is merged.

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

Test Plan: The code for the Python tests has been verified as part of this CI workflow.

Checklist:

Docs included if any changes are user facing

andreyvelich

Thank you so much for taking this @tariq-hasan and sorry for the late review!
I left a few comments.

andreyvelich · 2024-05-24T14:39:24Z

sdk/python/v1beta1/kubeflow/katib/api/katib_client.py

+                import logging
+                logging.basicConfig()
+                log = logging.getLogger("kubeflow.katib.api.katib_client")
+                log.setLevel(logging.DEBUG)


Thank you for adding this!

andreyvelich · 2024-05-24T14:40:34Z

sdk/python/v1beta1/kubeflow/katib/api/katib_client.py

                constants.KUBEFLOW_GROUP,
                constants.KATIB_VERSION,
                namespace,
                constants.EXPERIMENT_PLURAL,
                experiment,
            )
-            experiment_name = outputs["metadata"][
-                "name"
-            ]  # if "generate_name" is used, "name" gets a prefix from server


We need to keep that in case experiment is created using generate_name

Got it. My understanding of the code was not complete hence I removed it incorrectly. I have reverted this code change and updated the create_namespaced_custom_object_response function to account for the output of the create_namespaced_custom_object function.

andreyvelich · 2024-05-24T14:43:53Z

sdk/python/v1beta1/kubeflow/katib/api/katib_client_test.py

+            "experiment": create_experiment(name="experiment-mnist-ci-test"),
+            "namespace": "test",
+        },
+        "success",


Can we make this string as constant ?

I created a constant to replace "success".

andreyvelich · 2024-05-24T14:44:32Z

sdk/python/v1beta1/kubeflow/katib/api/katib_client_test.py

+            "experiment": create_experiment(
+                name="experiment-mnist-ci-test",
+                generate_name="experiment-mnist-ci-test",
+            ),
+            "namespace": "test"


If we pass generate_name and name how does Kubernetes Python client create the Experiment ?

I suppose that name is used when both name and generate_name are provided.
Ref: https://github.com/tariq-hasan/katib/blob/master/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py#L102-L105.

Would you suggest making changes to this case?

These parameters are mutually exclusive. Users can set name or generate_name

Okay. I have removed the two test cases where both name and generate_name are specified.

andreyvelich · 2024-05-24T14:47:35Z

sdk/python/v1beta1/kubeflow/katib/utils/utils.py

+        logger.debug(f"Current Optimal Trial:\n {experiment.status.current_optimal_trial}")
+        logger.debug(f"Experiment conditions:\n {experiment.status.conditions}")


Do we want to keep print here similar to get_job_logs API in Training Client: https://github.com/kubeflow/training-operator/blob/master/sdk/python/kubeflow/training/api/training_client.py#L1193 ?
Since when user runs this API, the SDK works like CLI and user wants to get output.
WDYT @droctothorpe @tariq-hasan @kubeflow/wg-automl-leads ?

I have reverted the changes to keep print.

andreyvelich · 2024-06-12T16:35:13Z

Hi @tariq-hasan, do you have some time to finish this PR ?

tariq-hasan · 2024-06-14T20:26:55Z

Hi @tariq-hasan, do you have some time to finish this PR ?

Hi @andreyvelich I will try to complete it this week. Sorry about the delay.

Signed-off-by: tariq-hasan <mmtariquehsn@gmail.com>

…ructor Signed-off-by: tariq-hasan <mmtariquehsn@gmail.com>

andreyvelich

Thanks for the update, only 1 comment from me @tariq-hasan.
/assign @johnugeorge @tenzen-y

andreyvelich · 2024-06-17T12:34:29Z

sdk/python/v1beta1/kubeflow/katib/constants/constants.py

@@ -56,3 +57,6 @@
 BASE_IMAGE_MXNET = "docker.io/mxnet/python:1.9.1_native_py3"

 DEFAULT_DB_MANAGER_ADDRESS = "katib-db-manager.kubeflow:6789"
+
+# Test result constants
+TEST_RESULT_SUCCESS = "success"


Please keep this variable in the katib_client_test.py since we use it only in that file.

Signed-off-by: tariq-hasan <mmtariquehsn@gmail.com>

andreyvelich

Thank you for this great contribution @tariq-hasan 🎉
/lgtm
/approve

google-oss-prow · 2024-06-20T15:33:39Z

[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:

~~OWNERS~~ [andreyvelich]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tariq-hasan · 2024-06-21T03:08:51Z

Thank you for this great contribution @tariq-hasan 🎉 /lgtm /approve

I suppose that the next step is to write unit test for the tune function.

andreyvelich · 2024-06-24T13:56:57Z

Thank you for this great contribution @tariq-hasan 🎉 /lgtm /approve

I suppose that the next step is to write unit test for the tune function.

That's right @tariq-hasan. Please feel free to create an issue and assign it to yourself.

Additionally, it would be nice to add e2e test for tune API, like for create_experiment here: https://github.com/kubeflow/katib/blob/master/test/e2e/v1beta1/scripts/gh-actions/run-e2e-experiment.py#L172
Similar to Training Operator tests: https://github.com/kubeflow/training-operator/blob/master/sdk/python/test/e2e/test_e2e_pytorchjob.py#L174

…ubeflow#2325) * added logger for katib_client module Signed-off-by: tariq-hasan <mmtariquehsn@gmail.com> * added API_VERSION as a constant Signed-off-by: tariq-hasan <mmtariquehsn@gmail.com> * updated the KatibClient constructor to match the TrainingClient constructor Signed-off-by: tariq-hasan <mmtariquehsn@gmail.com> * added test for create_experiment in katib_client Signed-off-by: tariq-hasan <mmtariquehsn@gmail.com> --------- Signed-off-by: tariq-hasan <mmtariquehsn@gmail.com>

google-oss-prow bot added the size/L label May 6, 2024

google-oss-prow bot requested review from andreyvelich, anencore94 and johnugeorge May 6, 2024 05:39

tariq-hasan changed the title ~~Add unit tests for katib client~~ Add unit test for create_experiment in the katib_client module May 6, 2024

tariq-hasan force-pushed the add-unit-tests-for-katib-client branch from 1545807 to 2aacab9 Compare May 6, 2024 06:10

andreyvelich reviewed May 24, 2024

View reviewed changes

tariq-hasan force-pushed the add-unit-tests-for-katib-client branch 3 times, most recently from c4e9bf4 to 4e963bc Compare June 15, 2024 18:49

tariq-hasan added 3 commits June 15, 2024 14:52

added logger for katib_client module

67c7e74

Signed-off-by: tariq-hasan <mmtariquehsn@gmail.com>

added API_VERSION as a constant

a817171

Signed-off-by: tariq-hasan <mmtariquehsn@gmail.com>

updated the KatibClient constructor to match the TrainingClient const…

7c03cb4

…ructor Signed-off-by: tariq-hasan <mmtariquehsn@gmail.com>

tariq-hasan force-pushed the add-unit-tests-for-katib-client branch 3 times, most recently from 2092eec to ab56bf9 Compare June 15, 2024 19:40

google-oss-prow bot added size/XXL and removed size/L labels Jun 15, 2024

tariq-hasan force-pushed the add-unit-tests-for-katib-client branch from ab56bf9 to 85dc9a1 Compare June 15, 2024 19:41

google-oss-prow bot added size/L and removed size/XXL labels Jun 15, 2024

andreyvelich reviewed Jun 17, 2024

View reviewed changes

google-oss-prow bot assigned johnugeorge and tenzen-y Jun 17, 2024

andreyvelich mentioned this pull request Jun 17, 2024

[SDK] Fix empty list for env variables and numpy version #2360

Merged

added test for create_experiment in katib_client

6dbea78

Signed-off-by: tariq-hasan <mmtariquehsn@gmail.com>

tariq-hasan force-pushed the add-unit-tests-for-katib-client branch from 887a69b to 6dbea78 Compare June 18, 2024 22:31

Merge branch 'master' into add-unit-tests-for-katib-client

c1c24c4

andreyvelich approved these changes Jun 20, 2024

View reviewed changes

google-oss-prow bot assigned andreyvelich Jun 20, 2024

google-oss-prow bot added the lgtm label Jun 20, 2024

google-oss-prow bot added the approved label Jun 20, 2024

google-oss-prow bot merged commit 99ba1d5 into kubeflow:master Jun 20, 2024
60 checks passed

andreyvelich mentioned this pull request Jun 20, 2024

[WIP] add unit tests for katib sdk #2305

Closed

1 task

andreyvelich mentioned this pull request Jun 24, 2024

[SDK]Support Docker image as objective in the tune API #2338

Open

1 task

tariq-hasan mentioned this pull request Jun 24, 2024

Add unit test for tune in the katib_client module #2372

Closed

tariq-hasan deleted the add-unit-tests-for-katib-client branch June 29, 2024 17:49

tariq-hasan mentioned this pull request Jul 14, 2024

Add e2e test for tune in the katib_client module #2383

Closed

Electronic-Waste mentioned this pull request Jul 26, 2024

[SDK] grpc-related bugs in Python SDK #2395

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit test for `create_experiment` in the `katib_client` module #2325

Add unit test for `create_experiment` in the `katib_client` module #2325

tariq-hasan commented May 6, 2024 •

edited

Loading

andreyvelich left a comment

andreyvelich May 24, 2024

andreyvelich May 24, 2024

tariq-hasan Jun 15, 2024

andreyvelich May 24, 2024

tariq-hasan Jun 15, 2024

andreyvelich May 24, 2024

tariq-hasan Jun 15, 2024 •

edited

Loading

andreyvelich Jun 15, 2024

tariq-hasan Jun 15, 2024

andreyvelich May 24, 2024

tariq-hasan Jun 15, 2024

andreyvelich commented Jun 12, 2024

tariq-hasan commented Jun 14, 2024

andreyvelich left a comment

andreyvelich Jun 17, 2024

andreyvelich left a comment

google-oss-prow bot commented Jun 20, 2024

tariq-hasan commented Jun 21, 2024

andreyvelich commented Jun 24, 2024 •

edited

Loading

		logger.debug(f"Current Optimal Trial:\n {experiment.status.current_optimal_trial}")
		logger.debug(f"Experiment conditions:\n {experiment.status.conditions}")

Add unit test for create_experiment in the katib_client module #2325

Add unit test for create_experiment in the katib_client module #2325

Conversation

tariq-hasan commented May 6, 2024 • edited Loading

andreyvelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tariq-hasan Jun 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich commented Jun 12, 2024

tariq-hasan commented Jun 14, 2024

andreyvelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Jun 20, 2024

tariq-hasan commented Jun 21, 2024

andreyvelich commented Jun 24, 2024 • edited Loading

Add unit test for `create_experiment` in the `katib_client` module #2325

Add unit test for `create_experiment` in the `katib_client` module #2325

tariq-hasan commented May 6, 2024 •

edited

Loading

tariq-hasan Jun 15, 2024 •

edited

Loading

andreyvelich commented Jun 24, 2024 •

edited

Loading