-
Notifications
You must be signed in to change notification settings - Fork 816
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 bug for fq_name #2208
fix bug for fq_name #2208
Conversation
@Qianh1225 thanks for the PR~! Can you run the pre-commit hook to unblock the merge? |
Testing[4020636] @ 6d69956 |
Let's add the new tests internally and wait for them to succeed as well. |
Do you want to me add test for testing this code? Can you be more specific? |
I meant adding of the new UX tests to test deployment of flows with deploy time and static triggers. |
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.
Change seems good to go. Would like some more details on what the actual issue was that this caused though.
Testing[942] @ 6d69956 had 4 FAILUREs. |
Testing[4020636] @ 8195ab3 |
Testing[4020636] @ 7e94fcf |
Testing[4020636] @ 45c6a2f |
Testing[942] @ 45c6a2f had 3 FAILUREs. |
Testing[942] @ 7e94fcf had 4 FAILUREs. |
raise MetaflowException( | ||
"The *project_branch* attribute of the *flow* is not a string" | ||
) | ||
trigger = result | ||
if isinstance(trigger, dict): |
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.
maybe add a comment on this code: "effect is to set all fields to None if they don't exist.
Testing[4020636] @ 7dfa81e |
Testing[942] @ 7dfa81e had 4 FAILUREs. |
|
Testing[4020636] @ ef5c89e |
trigger = deploy_time_eval(trigger) | ||
if is_stringish(trigger): | ||
pass | ||
elif isinstance(trigger, dict): |
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.
This logic needs further refactoring in a follow up PR.
) | ||
if "project_branch" in trigger: | ||
if is_stringish(trigger["project_branch"]): | ||
result["project_branch"] = trigger["project_branch"] |
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.
Can you extract out lines 602-630 into a helper function? The code is identical to lines 397-429. Also on line 425 it's setting result to branch
instead of project_branch
. Which one is correct?
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.
Is it supposed to be branch
or project_branch
? Do we need to update line 425? There are a few other places it's using branch
in the file
Testing[942] @ ef5c89e had 5 FAILUREs. |
@@ -478,7 +481,7 @@ def flow_init( | |||
) | |||
if "project_branch" in flow: | |||
if is_stringish(flow["project_branch"]): | |||
result["branch"] = flow["project_branch"] | |||
result["project_branch"] = flow["project_branch"] |
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.
this change seems to actually fix a bug with the argo events implementation that has gone unnoticed. Currently deploying a flow with
@trigger_on_finish(flow={"name": "ProjectEventsTestFlow", "project": "sa_test_project", "project_branch": "user.saikonen"})
does not apply a filter on the project_branch though it should.
with the introduced changes this is correctly being applied. The relevant code is in https://github.com/Netflix/metaflow/blob/master/metaflow/plugins/argo/argo_workflows.py#L641-L646
cc @savingoyal
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.
I'm not sure if this is a regression or not. have triggers filtered by project&branch been working in the past with argo?
if is_stringish(trigger): | ||
pass | ||
elif isinstance(trigger, dict): | ||
if "name" not in trigger: |
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.
There is an interesting fix that this PR introduces. Previously this incorrect usage was also working for whatever reason:
def flow_name_dict_func(ctx):
# Is 'flow' correct??? docstring says 'name'
return {"flow": "DeployTimeTriggerParams"}
@trigger_on_finish(flow=flow_name_dict_func)
class DeployTimeTriggerOnFinishFlow4(FlowSpec):
With the changes in this PR, Metaflow correctly raises an exception on the missing name
in the dict
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.
There is an issue with the current PR though when trying out with the correct syntax.
def flow_name_dict_func(ctx):
# Is 'flow' correct??? docstring says 'name'
return {"name": "DeployTimeTriggerParams"}
# return {"name": "DeployTimeTriggerParams", "project": "TEST", "project_branch": "test_branch"}
@trigger_on_finish(flow=flow_name_dict_func)
class DeployTimeTriggerOnFinishFlow4(FlowSpec):
fails with KeyError
on argo workflows due to event["flow"]
not being set.
Testing[942] @ 8195ab3 had 4 FAILUREs. |
superseded by #2218 |
The bug cause the issue when user use the @trigger_on_finish. Previously, it is not possible to deploy flows with
@trigger_on_finish
decorator whose event name was a string.Deploying this to our internal scheduler would fail because in
flow_init
we would convertexample_flow
to a fully qualified name (fq_name
) here and when parsing the triggers during graph creation, we would read the value asname
instead offq_name
. This PR updates that and the logic for parsing deploy time triggers.The code can be further refactored for readability and @Qianh1225 will push a follow up PR for that.