[SDK] improve PVC creation name error #2496

mahdikhashan · 2025-01-17T13:28:05Z

What this PR does / why we need it:
This PR handles potential name errors for PVCs gracefully.

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 #2491

Checklist:

Docs included if any changes are user facing

helenxie-bit

Thanks for the contribution! Basically LGTM, just a small comment.

helenxie-bit · 2025-01-17T19:57:28Z

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

+    # RFC 1123 regex for valid PVC names: lowercase alphanumeric, '-', or '.'.
+    return bool(
+        re.match(
+            r"^[a-z0-9]([a-z0-9\-]*[a-z0-9])?(\.[a-z0-9]([a-z0-9\-]*[a-z0-9])?)*$", name


Would using the same regex format as shown in the error message improve readability and maintainability?

would you please elaborate what you mean here?

I mean using this regex format in the ValueError message: '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*', since it took me a while to compare if they stand for the same thing. Or is there specific reason you changed the format a little bit?

ok, thanks for bringing this to my attention. shall i add any unit test for it? please let me know.

also, there are two failing ci tests, i'm guessing they are flaky tests, would there be any problem that this change have caused?

Since the unit tests for tune API is still under review, I think you can add your unit test after that one is merged.

I think the CI test failures are caused by resource problems. I've rerun the tests once, but one of them still failed due to network connectivity issue. Maybe we can try running them again later.

thanks for your time and help in this matter.

helenxie-bit · 2025-01-21T07:41:23Z

/rerun-all

helenxie-bit · 2025-01-21T20:57:32Z

/rerun-all

mahdikhashan · 2025-01-24T08:38:51Z

/rerun-all

mahdikhashan · 2025-01-24T13:53:29Z

/rerun-all

helenxie-bit · 2025-01-24T19:14:38Z

Thanks for the contribution!

/lgtm
/rerun-all

mahdikhashan · 2025-01-24T19:16:21Z

Thanks for the contribution!

/lgtm /rerun-all

is it fine if i add a few unit tests after the main unit test pr got merged - keeping this pr open till then?

helenxie-bit · 2025-01-24T19:16:30Z

/assign @tenzen-y @andreyvelich

helenxie-bit · 2025-01-24T19:21:20Z

Thanks for the contribution!
/lgtm /rerun-all

is it fine if i add a few unit tests after the main unit test pr got merged - keeping this pr open till then?

The main unit test is already approved, but it seems the CI test is still in progress. I think we can merge this PR first, and it would be better to open a new PR to add unit tests for this.

andreyvelich · 2025-01-24T19:23:52Z

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

@@ -557,6 +557,20 @@ class name in this argument.
            # Create PVC for the Storage Initializer.
            # TODO (helenxie-bit): PVC Creation should be part of Katib Controller.
            try:
+                if not utils.is_valid_pvc_name(name):


I am wondering what is the goal to introduce additional validation on top of Kubernetes default validation ?
Are we trying to make this message more user friendly ?
cc @kubeflow/wg-training-leads

Yes, I think that's the point.

yes, basically my goal was to make the message user friendlier. i'm open to suggestions.

Can we reconcile this error similar to 409 (CRD is already exist), so we won't introduce additional validation ?

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

Lines 138 to 143 in 05dbea6

except Exception as e:

if hasattr(e, "status") and e.status == 409:

raise Exception(

f"A Katib Experiment with the name "

f"{namespace}/{experiment_name} already exists."

)

yes, you are right - i'll change it.

…h correct name example Signed-off-by: mahdikhashan <[email protected]>

Signed-off-by: mahdikhashan <[email protected]>

mahdikhashan · 2025-01-27T18:51:55Z

Hi @mahdikhashan, do you have time to work on this PR before the RC.0 that we want to cut in 2 days ? Otherwise, we can include this feature in RC.1

done. i kindly ask for your review.

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

helenxie-bit · 2025-01-27T19:34:16Z

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

@@ -569,6 +569,11 @@ class name in this argument.
                    ),
                )
            except Exception as e:
+                if hasattr(e, "status") and e.status == 422:


I'm thinking we should move this check after this line since the error belongs to that scenario:

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

Line 582 in 40e1e65

else:

Additionally, to make the error easier to understand, we could tweak the error message a bit. How about this:

if hasattr(e, "status") and e.status == 422: raise ValueError( f"An Experiment with the name {name} is not valid: the name must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character." ) else: raise RuntimeError(f"failed to create PVC. Error: {e}")

I agree with the ValueError message (with a bit of change), with combining it with else, not really - since then we need to wait for this steps first:

pvc_list = self.core_api.list_namespaced_persistent_volume_claim( namespace=namespace ) # Check if the PVC with the specified name exists. for pvc in pvc_list.items: if pvc.metadata.name == name: print( f"PVC '{name}' already exists in namespace " f"{namespace}." ) break

so my idea is the function fails fast when the name is invalid then if its valid, continues with the check for the existence of the name. WDYT?

That makes sense.

Signed-off-by: mahdikhashan <[email protected]>

helenxie-bit · 2025-01-27T22:05:42Z

Thanks for the contribution!

/lgtm
/assign @kubeflow/wg-automl-leads @Electronic-Waste

andreyvelich · 2025-01-27T22:16:16Z

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

+                        f"alphanumeric characters ('a-z', '0-9'), hyphens ('-'), or periods ('.'). "
+                        f"It must also start and end with an alphanumeric character."
+                    )
+
                pvc_list = self.core_api.list_namespaced_persistent_volume_claim(


Should we also simplify this logic similar to this one:

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

Lines 140 to 143 in d6c7319

raise Exception(

f"A Katib Experiment with the name "

f"{namespace}/{experiment_name} already exists."

)

E.g. if status_code is 409 we just print that PVC already exists.

@helenxie-bit @mahdikhashan ?

If we add more details to the error message, it’ll make it easier for users to understand, which is the goal of this PR. But you’re right—Kubernetes API will also return detailed error reasons. So it depends on whether we want to keep the error messages consistent across the board.

My opinion is a bit more leaned to Helens. But I'm open to any changes.

I just meant that all of these:

pvc_list = self.core_api.list_namespaced_persistent_volume_claim( namespace=namespace ) # Check if the PVC with the specified name exists. for pvc in pvc_list.items: if pvc.metadata.name == name: print( f"PVC '{name}' already exists in namespace " f"{namespace}." ) break else: raise RuntimeError(f"failed to create PVC. Error: {e}")

can be replaced to

elif hasattr(e, "status") and e.status == 409: print(f"PVC '{name}' already exists in namespace " f"{namespace}.") else: raise RuntimeError(f"failed to create PVC. Error: {e}")

Does it make sense ?

Oh I see, SGTM 😄

yes, agreed.

Signed-off-by: mahdikhashan <[email protected]>

andreyvelich

Thanks for this contribution @mahdikhashan!
/lgtm
/approve

google-oss-prow · 2025-01-27T23:11:35Z

[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

google-oss-prow bot added the do-not-merge/work-in-progress label Jan 17, 2025

google-oss-prow bot requested review from Electronic-Waste and helenxie-bit January 17, 2025 13:28

google-oss-prow bot added size/L size/S and removed size/L size/S labels Jan 17, 2025

mahdikhashan force-pushed the improve-pvc-error-message branch from 7036b29 to e005825 Compare January 17, 2025 13:45

google-oss-prow bot added size/S and removed size/L labels Jan 17, 2025

mahdikhashan marked this pull request as ready for review January 17, 2025 13:48

google-oss-prow bot removed the do-not-merge/work-in-progress label Jan 17, 2025

google-oss-prow bot requested a review from anencore94 January 17, 2025 13:48

helenxie-bit reviewed Jan 17, 2025

View reviewed changes

andreyvelich added this to the v0.18 milestone Jan 20, 2025

Electronic-Waste mentioned this pull request Jan 21, 2025

[Release] Katib 0.18 Roadmap #2386

Open

7 tasks

google-oss-prow bot assigned helenxie-bit Jan 24, 2025

google-oss-prow bot added the lgtm label Jan 24, 2025

google-oss-prow bot assigned andreyvelich and tenzen-y Jan 24, 2025

andreyvelich reviewed Jan 24, 2025

View reviewed changes

google-oss-prow bot removed the lgtm label Jan 27, 2025

mahdikhashan force-pushed the improve-pvc-error-message branch from c74b4d6 to 3273e0f Compare January 27, 2025 18:38

google-oss-prow bot added size/XXL and removed size/S labels Jan 27, 2025

mahdikhashan added 5 commits January 27, 2025 19:40

improve pvc name error message by failing early and clear message wit…

ff3de2b

…h correct name example Signed-off-by: mahdikhashan <[email protected]>

fix lint

eea6de6

Signed-off-by: mahdikhashan <[email protected]>

fix lint

33028c1

Signed-off-by: mahdikhashan <[email protected]>

raise value error for wrong name format by reconciliation

516b4b5

Signed-off-by: mahdikhashan <[email protected]>

revert created utils

651b231

Signed-off-by: mahdikhashan <[email protected]>

mahdikhashan force-pushed the improve-pvc-error-message branch from 3273e0f to 651b231 Compare January 27, 2025 18:40

google-oss-prow bot added size/S and removed size/XXL labels Jan 27, 2025

mahdikhashan requested review from andreyvelich and helenxie-bit January 27, 2025 18:48

helenxie-bit reviewed Jan 27, 2025

View reviewed changes

mahdikhashan added 2 commits January 27, 2025 21:07

improve test case name

86a1841

Signed-off-by: mahdikhashan <[email protected]>

improve value error message

d6c7319

Signed-off-by: mahdikhashan <[email protected]>

mahdikhashan requested a review from helenxie-bit January 27, 2025 20:07

google-oss-prow bot added the lgtm label Jan 27, 2025

andreyvelich reviewed Jan 27, 2025

View reviewed changes

improve code flow

7a03bce

Signed-off-by: mahdikhashan <[email protected]>

google-oss-prow bot removed the lgtm label Jan 27, 2025

mahdikhashan requested a review from andreyvelich January 27, 2025 22:35

andreyvelich reviewed Jan 27, 2025

View reviewed changes

google-oss-prow bot added the lgtm label Jan 27, 2025

google-oss-prow bot added the approved label Jan 27, 2025

google-oss-prow bot merged commit 09523cd into kubeflow:master Jan 28, 2025
63 checks passed

	except Exception as e:
	if hasattr(e, "status") and e.status == 409:
	raise Exception(
	f"A Katib Experiment with the name "
	f"{namespace}/{experiment_name} already exists."
	)

[SDK] improve PVC creation name error #2496

[SDK] improve PVC creation name error #2496

Conversation

mahdikhashan commented Jan 17, 2025

helenxie-bit 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

helenxie-bit Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helenxie-bit commented Jan 21, 2025

helenxie-bit commented Jan 21, 2025

mahdikhashan commented Jan 24, 2025

mahdikhashan commented Jan 24, 2025

helenxie-bit commented Jan 24, 2025

mahdikhashan commented Jan 24, 2025

helenxie-bit commented Jan 24, 2025

helenxie-bit commented Jan 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahdikhashan Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahdikhashan commented Jan 27, 2025

Choose a reason for hiding this comment

mahdikhashan Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helenxie-bit commented Jan 27, 2025

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

Choose a reason for hiding this comment

andreyvelich left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Jan 27, 2025

helenxie-bit Jan 21, 2025 •

edited

Loading

mahdikhashan Jan 24, 2025 •

edited

Loading

mahdikhashan Jan 27, 2025 •

edited

Loading