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

Fix params passing to TaskFlow tasks run with CLI #28667

Closed

Conversation

vemikhaylov
Copy link
Contributor

Params passed to the TaskFlow tasks using the CLI airflow tasks test command aren't worked correctly (see the linked issue for the more details and examples). This PR is to fix it.

closes: #28287


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@vemikhaylov vemikhaylov force-pushed the 28287-task-flow-cli-test-params branch from 9e4f321 to 09e0ca5 Compare January 1, 2023 22:45
Comment on lines +564 to +566
if isinstance(task, DecoratedOperator):
task.op_args = []
task.op_kwargs.update(passed_in_params)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sure that this is the best place to override this. Probably the args should rather be overridden somewhere closer to execute (e.g., https:/apache/airflow/blob/main/airflow/operators/python.py#L174).

I lack clear understanding what params are / should be used for as opposed to op_args and op_kwargs, so overriding here looked like a safer change to me.

@@ -30,18 +30,15 @@


@task(task_id="run_this")
def my_py_command(params, test_mode=None, task=None):
Copy link
Contributor Author

@vemikhaylov vemikhaylov Jan 1, 2023

Choose a reason for hiding this comment

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

It wasn't clear to me if a specific case was intended to be tested here originally with the usage of the params name here, which is also a name of the task.params attribute or it's just a coincidence.

I moved it to a simpler structure. If the original one made sense and is needed as well, please let me know, I'll restore it.

@vemikhaylov vemikhaylov marked this pull request as ready for review January 1, 2023 23:01
@uranusjr
Copy link
Member

uranusjr commented Jan 3, 2023

I don’t think this is correct. Params are params, and different from arguments to the task function. You can access params in a taskflow task like this:

@task
def my_task(params=None):
    # At runtime 'params' holds the params.

@potiuk
Copy link
Member

potiuk commented Jan 3, 2023

Agree with @uranusjr

@potiuk potiuk closed this Jan 3, 2023
@vemikhaylov
Copy link
Contributor Author

Hey @uranusjr and @potiuk! Thank you a lot for the feedback and providing a link to the docs, I've learned something new!

So from what you mentioned above, if I understand it correctly, params seems to be a different parallel concept to the tasks args and the current behaviour is correct, right?

Is the issue #28287 not completely correct and not a bug, but rather a feature request then? In other words, do we still want to have a way to pass args to a task run with the CLI command airflow tasks test (e.g., passing --task-kwargs)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Testing tasks from CLI when they need dependencies from other tasks
3 participants