Treat infer_artifact=False as the documented default in add_tags/remove_tags#5097
Treat infer_artifact=False as the documented default in add_tags/remove_tags#5097chuenchen309 wants to merge 2 commits into
Conversation
…ve_tags The public overloads of `add_tags` and `remove_tags` type `infer_artifact` as `bool = False`, so passing `False` explicitly is documented and valid. But the implementations default it to `None` and every dispatch branch tests `infer_artifact is None`, so an explicit `False` matches no branch and falls through to a `ValueError: Unsupported way to call ...` — even when other valid identifiers (e.g. `run=...`) are supplied. Normalize `False` to `None` at the top of both functions so it dispatches identically to omitting the argument (both mean "don't infer"). The `infer_artifact is True` inference path is unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Hey @chuenchen309, thank you for your contribution. We'll take a look right away. |
| # `infer_artifact=False` is the documented default of the public overload | ||
| # and means "don't infer" — same as omitting it. The dispatch below tests | ||
| # `infer_artifact is None`, so normalize False to None to avoid falling | ||
| # through to the "unsupported call" error. |
There was a problem hiding this comment.
I think we should be a bit stricter here.
infer_artifact is only meaningful for the artifact versions with:
add_tags(tags=[...], infer_artifact=True)
add_tags(tags=[...], artifact_name=..., infer_artifact=True)For manual resource like run, pipeline, artifact, infer_artifact=False is not really part of the API shape. Silently normalizing it to None at the top makes irrelevant argument combinations succeed which would go against the validation.
Could we instead only handle False in the call shapes where this overload/default is meaningful, and keep the unsupported call error for combinations like add_tags(run=..., infer_artifact=False)?
In other words, I agree that False means “do not infer", but I don’t think that should automatically make it valid for every path.
bcdurak
added
backend
2 participants
Describe changes
The public overloads of
add_tagsandremove_tagstypeinfer_artifactas
bool = False(src/zenml/utils/tag_utils.py), so passingFalseexplicitly is documented and valid. But the implementations default the
parameter to
Optional[bool] = None, and every dispatch branch testsinfer_artifact is None. An explicitFalsetherefore matches no branchand falls through to
ValueError: Unsupported way to call ..., even when avalid identifier such as
run=...is supplied:Falsesemantically means "don't infer" — the same as omitting the argument.This normalizes
FalsetoNoneat the top of both functions so it dispatchesidentically to the default. The
infer_artifact is Trueinference path isuntouched, and calls that omit the argument (or pass
None/True) arecompletely unaffected — the new lines are a no-op unless the value is exactly
False.Pre-requisites
developand the open PR is targetingdevelop.already-documented behavior of a public overload; no docs, dashboard,
template, or project changes are needed.
Types of changes
Tests
Added
test_add_and_remove_tags_with_infer_artifact_falseintests/integration/functional/utils/test_tag_utils.py, which tags and untags apipeline run via
run=...withinfer_artifact=False. It fails ondevelop(the
ValueErrorabove) and passes with this change. The fulltest_tag_utils.pyfile passes (4 tests);ruff,ruff format,mypy(single-file), and
typosare clean on both changed files.Notes
release-notes/no-release-noteslabel (externalcontributors lack label permissions). Could a maintainer add the appropriate
one? This is a small user-facing bug fix, so
release-notesseems right.Disclosure: this change was drafted with AI assistance (Claude). The diagnosis,
the fix, and every verification step above were reviewed and run before opening.