fix: replace bare except with ValueError in metadata_helpers.py - #13981
fix: replace bare except with ValueError in metadata_helpers.py#13981praneshnikhar wants to merge 2 commits into
Conversation
The get_or_create_* functions and get_or_create_context_with_type used bare except: clauses to catch "not found" errors from the ML Metadata store. This silently swallowed unrelated errors (network, serialization, memory) and fell through to the create path, risking duplicate/invalid MLMD entities. Replace bare except: with except ValueError: to only catch the specific "not found" exception raised by ml-metadata store methods. Signed-off-by: praneshnikhar <praneshnikhar@gmail.com>
|
@praneshnikhar: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @praneshnikhar. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/ok-to-test |
|
/retest |
hbelmiro
left a comment
hbelmiro
left a comment
There was a problem hiding this comment.
Could you please add unit tests for the four get-or-create functions? The store is passed as a parameter, so they're easy to test with a mock. For each function, cover at least:
- Get path: mock the get method to return an existing type/context — assert the put method is never called and the existing object is returned.
- Create path: mock the get method to raise the caught exception — assert the put method is called and the new object is returned.
- Unrelated exception propagates: mock the get method to raise a different exception (e.g.,
RuntimeError) — assert it propagates instead of silently falling through to the create path.
|
/retest |
hbelmiro
left a comment
hbelmiro
left a comment
There was a problem hiding this comment.
The PR description claims ml-metadata raises ValueError on not-found, but inspecting the actual ml-metadata 1.21.0 source (metadata_store.py:1104-1105) shows get_artifact_type documents:
Raises:
errors.NotFoundError: if no type exists.
errors.InternalError: if query execution fails.
errors.NotFoundError inherits from StatusError(Exception) — it is not a subclass of ValueError. Catching ValueError here will never match a not-found response from the store, so the create path becomes unreachable. Every call will propagate NotFoundError as an unhandled exception instead of falling through to create the type.
Description
The
get_or_create_*functions andget_or_create_context_with_typeinmetadata_helpers.pyused bareexcept:clauses to catch "not found" errors from the ML Metadata store. The ml-metadata library raisesValueErrorwhen a type or context is not found, but bareexcept:catches everything — includingTypeError,KeyError, network errors, memory errors — and silently falls through to the create path. This risks creating duplicate or invalid MLMD entities when unrelated errors occur.Change type
/kind bug
Changelog