Added meaningful error message for case when task method name is same as inbuilt/ time module method#55252
Conversation
suman-himanshu
requested review from
amoghrajesh,
ashb and
kaxil
as code owners
|
Hello maintainers, I have included time and builtins module checks while keeping the author's concern in mind. Please let me know if there are any other modules we should include. Thank you! |
| from airflow.sdk.definitions.taskgroup import TaskGroup | ||
|
|
||
| STANDARD_NAMES = set(dir(builtins)) | ||
| STANDARD_NAMES.update(dir(time)) |
There was a problem hiding this comment.
This isn't scalable. Why just the time module? What about the 100s of other modules in the standard library?
There was a problem hiding this comment.
I can't really see this approach working.
a) There is nothing wrong with calling a task sleep, or any of the other builtin names. This is allowed.
b) The issue is not that the task fn is called sleep, but that the task decorator is trying to be called inside the task function leads to a hard-to-debug error.
That second point is what we want to catch, not that the task name is redecorated
|
(Sorry to close this PR without discussion, but this approach just won't work, and it would almost certainly break a good portion of DAGs that work today. So this PR is a non-starter.) |
2 participants
Added meaningful error message for case when task method name is same as inbuilt/ time module method
Post running sample DAG provide on github issue -
closes #49875
^ 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.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.