-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix custom task run name rendering #15773
Conversation
CodSpeed Performance ReportMerging #15773 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me!
fc43d42
to
8dfcc06
Compare
da6cc86
to
cfb6c6e
Compare
4339df9
to
08bf7d2
Compare
# If the callable accepts a 'parameters' kwarg, pass the entire parameters dict | ||
if "parameters" in sig.parameters: | ||
task_run_name = task.task_run_name(parameters=parameters) | ||
else: | ||
# If it doesn't expect parameters, call it without arguments | ||
task_run_name = task.task_run_name() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we pass the parameters if the author of the callable requests parameters
MRE i was using
closes #15747
the issue (which #15770 does not address) is that the
TaskRunContext
may receive a non-resolved dict ofparameters
when it is created, and theTaskRunContext
is treated as immutable. this itself feels like an incongruence to address - but this PR is mostly concerned with the custom task run namewith CSTRO I'm not sure that we can move the future resolution before the creation / entrance of the
TaskRunContext
(happy to talk this through sync) while also correctly handlingUpstreamTaskError
exceptions, so in addition to fixing the templating syntax liketask_run_name="using {input[val]}"
by deferring the attempt to render the custom name, this PR proposes a new DX when providing a callabletask_run_name
that needs access to the renderedparameters
if the user provides a function that accepts a
parameters
dict, then we will populate it with the dict of parameters, which at that point is guaranteed to be resolvedif we are okay with this slight pivot, then I think we should deprecate / remove the
get_parameters
fromruntime.task_run
, since it does not currently work for cases where futures need to be resolved