Deploy time triggers #2133
Deploy time triggers #2133
Conversation
| @@ -312,6 +422,13 @@ def flow_init( | |||
| "The *project_branch* attribute of the *flow* is not a string" | |||
| ) | |||
| self.triggers.append(result) | |||
| elif callable(self.attributes["flow"]) and not isinstance( | |||
There was a problem hiding this comment.
nit: here and other similar place: we probably want to also allow DeployTimeField (ie: not raise incorrect type). I don't think it happens but in terms of logic, we are ok with a deploytimefield.
There was a problem hiding this comment.
We had this check in our old trigger code in metaflow-custom, so I kept it
| if isinstance(value, (str, int, float)): | ||
| return value | ||
| if isinstance(value, dict): | ||
| return {to_pod(k): to_pod(v) for k, v in value.items()} | ||
| if isinstance(value, (list, set, tuple)): | ||
| return [to_pod(v) for v in value] | ||
| if isinstance(value, DeployTimeField): |
There was a problem hiding this comment.
why does this happen? I am not a fan of double "deploy time eval" since technically that can result in different values thereby giving an incorrect information to the user.
metaflow/plugins/events_decorator.py
Outdated
| param_value, DeployTimeField | ||
| ): | ||
| new_param_value = DeployTimeField( | ||
| "param", [list, dict], None, param_value, False |
There was a problem hiding this comment.
nit: I'd call it "parameters" as well here since that is what the user is passing.
There was a problem hiding this comment.
Also, it seems parameters can be a tuple or a list or a dict (here you just added list and dict).
We do lose the check on tuple of size 2 here but maybe we get it back later.
metaflow/plugins/events_decorator.py
Outdated
| # Case were trigger is a function that returns a list of events | ||
| # Need to do this bc we need to iterate over list later | ||
| if isinstance(trigger, DeployTimeField): | ||
| if isinstance(deploy_time_eval(trigger), list): |
There was a problem hiding this comment.
Calling deploy_time_eval twice. Also, at the top level, something that is a deploy-time field will be:
- a string (so needs to be handled like line 96)
- a dictionary (single event in dict form)
- a list of the two things above (when it is pushed as events)
So for string, that is done (we create the proper entry and done). For dictionary, we may need further processing of the elements (so continue). For list, add each element as above.
metaflow/plugins/events_decorator.py
Outdated
| for trigger in self.triggers: | ||
| old_trigger = trigger | ||
| # Entire event is a function (returns either string or dict) | ||
| if isinstance(trigger, DeployTimeField): |
There was a problem hiding this comment.
This should not happen since we removed the DeployTimeField in the first step. Now we need to go inside each dict (we either have a fully resolved dict or a dict with some portions that are not yet evaluated).
metaflow/plugins/events_decorator.py
Outdated
| if isinstance(trigger_params, DeployTimeField): | ||
| trigger_params = deploy_time_eval(trigger_params) | ||
| try: | ||
| trigger_params = json.loads(trigger_params) |
There was a problem hiding this comment.
we shouldn't need this -- the user needs to return this appropriately.
There was a problem hiding this comment.
I think this happens when the user returns a dictionary, not sure why
KaylaSeeley
force-pushed
the
deploy_time_triggers
branch
from
6687a18
to
d7211ef
Compare
https://docs.google.com/document/d/1-g-hPK5XC7sPmExXqFhxPTVAwnlPYyNKnmWaYIKvxb8/edit?tab=t.0#heading=h.6lt86ol75edp