Feat/make auto emit events configurable from argo UI#2782
Feat/make auto emit events configurable from argo UI#2782tfurmston wants to merge 8 commits intoNetflix:masterfrom
Conversation
|
Hey @saikonen I've reopened the PR here.
For my understanding, when you are talking about CLI option you are talking about the CLI options in the Argo Workflows plugin, right? So the idea would be to allow certain CLI options in that plugin to become parameters in the workflow somehow? |
|
@saikonen How about we add a Boolean flag to the Argo Workflow CLI that when set adds this particular option as a parameter to the workflow? If the flag is set and the there is a nameclash with an existing parameter in the workflow, which I guess would be very unlikely anyway, we would just fail the workflow creation. If someone wanted this option set, they would have to update the parameter name in the workflow. Would be much less intrusive to the main codebase than the original approach. What do you think? |
b655e97 to
5ea7c67
Compare
Greptile SummaryThis PR re-introduces the feature from PR #2770 (merged then reverted) that exposes However, the PR description itself acknowledges the two root causes of the revert, and the current implementation does not appear to resolve either of them:
Confidence Score: 1/5
Important Files Changed
Reviews (3): Last reviewed commit: "Merge branch 'master' into feat/make_aut..." | Re-trigger Greptile |
| + [ | ||
| Parameter("auto_emit_argo_events") | ||
| .value(1 if self.auto_emit_argo_events else 0) | ||
| .description( | ||
| "Toggle used to control emission of events to Argo Events." | ||
| ) | ||
| ] |
There was a problem hiding this comment.
Parameter always added, regardless of Argo Events usage
The auto_emit_argo_events parameter is appended unconditionally to every Argo workflow's .arguments(), so it will appear in the Argo UI for all deployed flows — even those that have no Argo Events integration and where auto_emit_argo_events was False at deployment time.
This is the same root issue that caused the previous incarnation of this change (PR #2770) to be reverted: it "introduc[es] a new always-present parameter to flows on Argo that will be confusing to users".
If the intent is to expose the toggle only for flows that actually use Argo Events (or that were deployed with auto_emit_argo_events=True), the block should be guarded:
# Only expose the toggle when the workflow was deployed with Argo Events awareness
+ ([
+ Parameter("auto_emit_argo_events")
+ .value(1 if self.auto_emit_argo_events else 0)
+ .description(
+ "Toggle used to control emission of events to Argo Events."
+ )
+ ] if self.triggers or self.auto_emit_argo_events else [])Without this guard, every Metaflow user running on Argo will see an unfamiliar auto_emit_argo_events parameter that is irrelevant to their workflow, which re-introduces the exact problem that led to the prior revert.
| "run-id", | ||
| "task-id", | ||
| "runner-attribute-file", | ||
| "auto-emit-argo-events", |
There was a problem hiding this comment.
Argo-specific reserved name leaks into general parameters.py
Adding "auto-emit-argo-events" (and implicitly "auto_emit_argo_events" via the hyphen-to-underscore conversion on line 417) to the global reserved parameter list in parameters.py couples a general-purpose Metaflow module to an Argo Workflows-specific concept.
This was one of the two explicit reasons cited for reverting PR #2770: "adding argo-specific references to parameters.py is undesirable".
Consider keeping this reservation within the Argo plugin itself — for example, by overriding a _reserved_parameter_names hook in ArgoWorkflows (or its CLI) rather than hard-coding Argo-specific names in the shared parameters.py.
This is true, but at present the PR has been reopened as-is in order to discuss alternatives. As such, it is not suggested that this PR has addressed those issues, which I agree are valid points. Once we reach a consensus I will be happy to make the changes to address these points. |
| .description( | ||
| "Toggle used to control emission of events to Argo Events." | ||
| ) |
There was a problem hiding this comment.
Parameter description does not document valid values
The description "Toggle used to control emission of events to Argo Events." doesn't tell users which values are accepted. The decorator parser in decorators.py (extract_args_kwargs_from_decorator_spec) first attempts json.loads, then int, then float, falling back to a raw string. Most common inputs ("0", "1", "true", "false") parse correctly to falsy/truthy values, but a user who enters "no", "off", or "disabled" will end up with a non-empty string that evaluates as truthy — silently keeping event emission enabled despite their intent.
Updating the description to explicitly state the accepted values would prevent this confusion:
| .description( | |
| "Toggle used to control emission of events to Argo Events." | |
| ) | |
| .description( | |
| "Toggle used to control emission of events to Argo Events. Set to 1 to enable, 0 to disable." | |
| ) |
This PR is a re-opening of this PR, which was merged and then reverted.
The reason for the original PR being reverted can be found here. Essentially, because of the following:
Previous PR Description
It adds a parameter to Metaflow Argo Workflow templates that allows users to toggle whether to emit event information to Argo Events when submitting a workflow from the Argo Workflow UI.
This feature was discussed on the Metaflow Outerbounds Slack workspace here.