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

Update invalid arg from snippet in TaskFlow tutorial doc #21446

Merged
merged 2 commits into from
Feb 25, 2022

Conversation

josh-fell
Copy link
Contributor

@josh-fell josh-fell commented Feb 9, 2022

Doesn't really add anything to the documentation to substitute with a different arg either.
Updated from max_retries to retries and added to the following example in the doc since using retries when simply deserializing an output probably isn't a great use case to showcase using that arg. The next task tries to create a queue which seems like a more plausible place to use retries.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

@potiuk potiuk closed this Feb 9, 2022
@potiuk potiuk reopened this Feb 9, 2022
@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Feb 9, 2022
@github-actions
Copy link

github-actions bot commented Feb 9, 2022

The PR is likely ready to be merged. No tests are needed as no important environment files, nor python files were modified by it. However, committers might decide that full test matrix is needed and add the 'full tests needed' label. Then you should rebase it to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@@ -301,7 +301,7 @@ to a TaskFlow function which parses the response as JSON.
)


@task(max_retries=2)
Copy link
Member

Choose a reason for hiding this comment

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

I think the point here is to show that you can pass "operator" params to the task decorator, separately from the function arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed but I'm not sure it's adding anything in this section.

However, this has come up many times on Slack, StackOverflow, Discussions, etc., but I feel like a mini chapter on this in the tutorial is a good addition instead. (Something on my list I need to get to.) That way we can be really explicit about the functionality in TaskFlow.

Maybe even some TaskFlow "recipes" documentation.

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

This arg isn't invalid -- It's the rough equivalent of PythonOperator(max_retries=2, python_callable=x)

@josh-fell
Copy link
Contributor Author

This arg isn't invalid -- It's the rough equivalent of PythonOperator(max_retries=2, python_callable=x)

Passing that specific arg causes DAG parsing errors. For example, given this DAG:

@task(max_retries=3)
def get_stuff(ti=None):
    print(vars(ti))


@dag(start_date=datetime(2022, 1, 1), schedule_interval=None)
def test_stuff():
    get_stuff()


dag = test_stuff()

You get this error:
image

@ashb
Copy link
Member

ashb commented Feb 9, 2022

Hmmm, I think that's a bug in the python decorator then!

@josh-fell
Copy link
Contributor Author

I thought so too but I couldn't find "max_retries" anywhere that wasn't in the Amazon provider and/or pertinent to BaseOperator, PythonOperator, the decorators, and the like. Unless during my moonlighting I completely missed it.

retries, of course but not max_retries.

@josh-fell
Copy link
Contributor Author

If it is a bug, I'd much rather fix that than these docs.

@potiuk
Copy link
Member

potiuk commented Feb 9, 2022

I thought so too but I couldn't find "max_retries" anywhere that wasn't in the Amazon provider and/or pertinent to BaseOperator, PythonOperator, the decorators, and the like. Unless during my moonlighting I completely missed it.

I also have not found max_retries other than in providers, so I believe this was a mistake here.

@ashb
Copy link
Member

ashb commented Feb 9, 2022

🤦🏻 So yeah, this should just be @task(retries=3) then!

Still, the point being to show how you can pass baseoperator params on :)

@@ -312,7 +312,7 @@ The reverse can also be done: passing the output of a TaskFlow function as an in

.. code-block:: python

@task
@task(retries=3)
Copy link
Contributor Author

@josh-fell josh-fell Feb 9, 2022

Choose a reason for hiding this comment

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

I added retries to the next example. Didn't seem relevant to set a retry on a task that just deserializes an output. More likely to have retries creating a queue though which is what the next example shows.

Copy link
Member

Choose a reason for hiding this comment

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

Nice idea.

@josh-fell josh-fell changed the title Remove invalid arg from snippet in TaskFlow tutorial doc Update invalid arg from snippet in TaskFlow tutorial doc Feb 9, 2022
@josh-fell
Copy link
Contributor Author

@ashb WDYT about the new variation on the update to the snippet?

@ashb ashb merged commit 5b41e2d into apache:main Feb 25, 2022
@josh-fell josh-fell deleted the patch-1 branch February 25, 2022 19:49
@jedcunningham jedcunningham added the type:doc-only Changelog: Doc Only label Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:documentation okay to merge It's ok to merge this PR as it does not require more tests type:doc-only Changelog: Doc Only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants