Fix mypy errors in example DAGs #19918
Conversation
boring-cyborg
bot
added
area:providers
provider:Apache
provider:amazon-aws
|
CC @potiuk |
| delete_bucket = S3DeleteBucketOperator(task_id='s3_bucket_dag_delete', force_delete=True) | ||
| delete_bucket = S3DeleteBucketOperator( # type: ignore[call-arg] | ||
| task_id='s3_bucket_dag_delete', force_delete=True | ||
| ) | ||
|
|
There was a problem hiding this comment.
Chose to ignore the argument checking for each individual operator rather than the entire file (like several other example DAGs) because the check should be performed on the TaskFlow function. However, the TaskFlow function is unlikely to change so happy to apply ignore check to the entire file as well.
josh-fell
changed the title
| # Ignore mypy argument checking. Some operator args will be passed via ``default_args``. | ||
| # type: ignore[call-arg] |
There was a problem hiding this comment.
I think instead of ignoring like this, we should probably “fix” @task to make these not fail in the first place.
There was a problem hiding this comment.
We've got airflow.mypy.plugin.decorators doing some things already, so that would be the place to add that sort of check if we cant do it with type stubs etc.
There was a problem hiding this comment.
Yeah. I fully agree we should use the opportunity to improve our type hinting rather than disable it, as our goal is not to pass mypy but to get better type hinting and less errors :)
There is a class of errors that can be avoided if we do it (for example most of the None`s) - that actually hit our users in the recent months, so this might have some long-lasting "real-world" consequences.
There was a problem hiding this comment.
@ashb Thanks for pro tip!
A little confused though and probably a dumb question coming: The @task decorator doesn't apply in these example DAGs. Is this more related to BaseOperator? This is my first real foray into mypy madness so just trying to getting my bearings on a fix.
But If we did start adding type annotations on TaskFlow functions in example DAGs, mypy probably wouldn't complain during its arg checking, right? We'll have to set a default value for the arg in the function signature to avoid a TypeError when parsing the DAG in order to use default_args (as we would if we wanted to directly access context fields).
Mypy is quiet on something like this:
from datetime import datetime
from typing import Dict, Optional
from airflow.decorators import dag, task
@task
def blah(stuff: Optional[str] = None, other_stuff: str = "Nope."):
print(stuff)
print(other_stuff)
@dag(
start_date=datetime(2021, 12, 1),
schedule_interval=None,
default_args={"stuff": "Here is your stuff.", "other_stuff": "Mine."},
)
def blah_blah():
_blah = blah()
dag = blah_blah()There was a problem hiding this comment.
This one is really about not add "blanket" ignore for whole file, but trying to find better ways to deal with it. Maybe not yet complete, but a way that will be "simple" and easy to apply to get rid of all the current MyPy complaints.
There is a very interesting parallel discussion on validation of the taskflow args dynamically or at runtime: #19884. but I think we should not go there yet, we should just focus on what MyPy complains on currently.
We also have this one: #19886 by @uranusjr where we have a stub added to context so that the parameters in context are also type-hinted.
I am quite sure with some clever tricks we will be able to use MyPy to do more type validation on context params and inputs/outputs passed between tasks via task flows but, I think this should be next step after we fix all the current problems.
There was a problem hiding this comment.
This one is really about not add "blanket" ignore for whole file, but trying to find better ways to deal with it.
Yeah it felt icky to do it and to slap # type: ignore[call-args] on each operator. But I suppose if they felt icky, probably means I shouldn't have done it. Although maybe the latter is a "passable" interim approach until the other conversations/solutions you pointed out get fleshed out? If there is no urgent timeline on this then perhaps it is better to put in a solid fix rather than bandaids.
There was a problem hiding this comment.
Agree. We have a few parallel changes for that one and hopefully we'll find some good solutions for similar cases.
There was a problem hiding this comment.
Sorry @potiuk I worded my comment poorly. You agree that I should add the ignore[call-args] markers or I should hold off on this PR/defer to the better solution?
|
@josh-fell - still waiting on this or shall we close it? |
I had a poorly-worded question to you and wanted to know what you had agreed to above. Is it OK to move forward with adding the |
|
I think we should do proper-fixes then :) this is not a blocker at all - I just started to think that this one might be closed as soon as all the parallell "smaller" fixes are merged, because they will fix everything here ;) |
Yeah this might be something we tackle last (i.e. getting MyPy to "understand" |
|
Closing. We'll see if other fixes bubble up into alleviating these. If not, we can revisit. |
Part of #19891
^ 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.