Stop format_name_template from mutating the caller substitutions dict#5091
Closed
chuenchen309 wants to merge 1 commit into
Closed
Stop format_name_template from mutating the caller substitutions dict#5091chuenchen309 wants to merge 1 commit into
chuenchen309 wants to merge 1 commit into
Conversation
`substitutions or {}` only builds a new dict when the argument is falsy, so a
non-empty dict is aliased and then has date/time written into it:
subs = {"custom": "abc"}
format_name_template("run-{custom}-{date}-{time}", substitutions=subs)
subs # {'custom': 'abc', 'date': '2026_07_16', 'time': '16_50_23_522789'}
An empty dict masks this (`{} or {}` yields a new dict), which is likely why
it went unnoticed.
_create_snapshot passes pipeline_configuration.substitutions straight in when
deriving a schedule name, before the snapshot is serialized into the
PipelineSnapshotRequest, so the injected date/time get persisted. Because
finalize_substitutions uses setdefault, the baked-in values then win over the
real start time forever after:
cfg.finalize_substitutions(start_time=datetime(2027, 1, 1))["date"]
# '2026_07_16' -- the future start time is ignored
Copy the dict instead. Explicit date/time substitutions still take precedence,
and the returned name is unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe changes
format_name_templatemutates the caller'ssubstitutionsdict.substitutions or {}only builds a new dict when the argument is falsy, so a non-empty dict is aliased and then written into by the.update()/.setdefault()calls below:An empty dict masks the bug (
{} or {}yields a fresh dict), which is likely why it went unnoticed.Why it matters
_create_snapshotpassespipeline_configuration.substitutionsstraight in when deriving a schedule name (pipeline_definition.py:995-998), and that happens before the snapshot is serialized into thePipelineSnapshotRequest(:1085) — so the injecteddate/timeget persisted into the snapshot.finalize_substitutionsthen usessetdefault, so the baked-in values win over the real start time from that point on:Every run off that schedule then resolves the same frozen
{date}/{time}, i.e. the same run name — the collision the server warns about insql_zen_store.py. Triggering it needs nothing unusual: a schedule without an explicit name, a non-empty@pipeline(substitutions={...})(public, documented), and arun_name_templatecontaining{date}/{time}— which is the default (run_utils.py:73).Changes
One line: copy the dict. Explicit
date/timesubstitutions still take precedence, and the returned name is unchanged.Two tests added — the mutation guard fails on
developand passes here; the second pins that caller-supplied date/time are still honored.format_name_templatehad no test coverage, which is why this survived.Pre-requisites
developand the open PR is targetingdevelop.Types of changes
Verification
pytest tests/unit/utils/test_string_utils.py— 6 passed.ruff check(with--extend-ignore Dfor tests, perscripts/lint.sh) andruff format --checkclean.mypy src/zenml/utils/string_utils.pyreports one pre-existingunused-ignoreat line 252, unrelated to this change — it appears identically on a cleandevelop(verified viagit stash).xref: #5080 also touches
string_utils.py, but only adds a newappend_random_suffixhelper near the top of the file — no overlap withformat_name_template.I could not add a release-notes label myself (external contributors lack label permissions); this looks like
no-release-notesto me, but happy to defer.AI disclosure: drafted with Claude Code (Opus 4.8), including the root-cause trace and the tests. I ran the repro and the tests locally and reviewed the diff before opening.