Skip to content
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

AIP-8651 Include parameters with no defaults in WorkflowTemplate #311

Open
wants to merge 2 commits into
base: feature/aip
Choose a base branch
from

Conversation

cloudw
Copy link
Collaborator

@cloudw cloudw commented Aug 24, 2024

Parameters with no value is not valid for Workflows but are valid for WorkflowTemplate. Without being added to WorkflowTemplate, these parameter will not be accepted from Argo UI or sensors.

@cloudw cloudw requested a review from talebzeghmi August 24, 2024 04:35
@cloudw cloudw self-assigned this Aug 24, 2024
@cloudw cloudw force-pushed the yunw/AIP-8651-include-all-params branch from 4ef1ef0 to 73521f2 Compare August 24, 2024 04:36

import boto3
import pytest
from botocore.exceptions import ClientError
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused imports.

for param in self.flow._get_parameters()
if param not in flow_parameters
]
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this may not work:

File "/home/zservice/metaflow/metaflow/task.py", line 548, in run_step
    self._exec_step_function(step_func)
  File "/home/zservice/metaflow/metaflow/task.py", line 53, in _exec_step_function
    step_function()
  File "/home/zservice/metaflow/metaflow/plugins/aip/tests/flows/flow_triggering_flow.py", line 89, in start
    self.submit_template(path)
  File "/home/zservice/metaflow/metaflow/plugins/aip/tests/flows/flow_triggering_flow.py", line 198, in submit_template
    subprocess.run(
  File "/usr/local/lib/python3.9/subprocess.py", line 528, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['argo', 'template', '-n', 'metaflow-integration-testing-internal', 'create', '/tmp/wfdsk-ftf-test-argo-d64fe9ff-f1d7-4c7c-8551-c45c3c600063-0.yaml']' returned non-zero exit status 1.

see:

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: it'll also be important to ensure it's tested with a workflow compiled with an exit handler to ensure we don't hit argoproj/argo-workflows#6036

@cloudw cloudw force-pushed the yunw/AIP-8651-include-all-params branch from afef70b to ab6a17f Compare September 22, 2024 23:43
@cloudw cloudw force-pushed the yunw/AIP-8651-include-all-params branch from ab6a17f to 0738816 Compare October 2, 2024 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants