You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Taragolis (via GitHub)" <gi...@apache.org> on 2023/11/01 20:47:53 UTC
[PR] Add @final decorator to DAG class [airflow]
Taragolis opened a new pull request, #35360:
URL: https://github.com/apache/airflow/pull/35360
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
Make DAG as [final classe](https://docs.python.org/3/library/typing.html#typing.final), it is just helper decorator for static checks and IDE and could be use for inform end users that subclassing DAG class it is not intended behaviour.
`typing_extensions.final` uses here instead of `typing.final` due to https://bugs.python.org/issue46342
related: https://github.com/apache/airflow/discussions/31974#discussioncomment-6203057
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `@final` decorator to DAG class [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #35360:
URL: https://github.com/apache/airflow/pull/35360#issuecomment-1789716824
> This is a breaking change where it could fail the Mypy test for the users who already subclass the DAG class.
Not sure that static check could be counted as breaking change, runtime code would still work. I see also a complaining about migration to SA 2.0 compatible syntax also breaks someone static checks.
Anyway right now this change breaks our static checks 😒
> my team has a great experience with a custom DAG class used to simplify creating dags by multiple teams
I use the Factories for the same approach without inheritance, different users different use cases and expectations. Someone might expects that DAG methods could be overwritten.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `@final` decorator to DAG class [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35360:
URL: https://github.com/apache/airflow/pull/35360#issuecomment-1789737980
See my follow-up in https://github.com/apache/airflow/discussions/31974#discussioncomment-6203057
My point is not that DAG should be final, but when you extend it you need to understand what you are doing. `dag.py` has 4000+ lines and there is quite a bit of dark-python-magic in. Simply the thought that you can extend DAG without understanding how it all works (with decorators using it, mapped tasks, metaclass handing apply_defaults, 9 dunder methods, SerializedDAG extending from it and plenty of other things you see when you open `dag.py` file is well, SCARY.
You don't extend such huge class with metaclasses, about 150 metods, 9 dunder methods without REALLY knowing what you are doing.
And the user seem to be complaining that we have no documentation that explains that. So yes, while I think maybe a documentation is needed, the I think the users should contribute it - those who want to extend it, have a good reason and explain the use case and implement it (maybe with our help) AND CONTRIBUTE THE DOCS BACK.
I don't think WE (maybe @hussein-awala who did it as a user is) should be doing it - because this should really come from the needs of our users on how they are extending it. As maintainers - if we have not foreseen the ways DAG can be extended , then it means it is not meant to be extended. But if our users have a need and do it and achieve something good with it - it's cool if they contribute it back (which BTW we might find as a good idea to not document but incorporate in our DAG implementation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `@final` decorator to DAG class [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis closed pull request #35360: Add `@final` decorator to DAG class
URL: https://github.com/apache/airflow/pull/35360
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `@final` decorator to DAG class [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #35360:
URL: https://github.com/apache/airflow/pull/35360#issuecomment-1789693925
Ahhhh.... SerializedDAG 🙄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `@final` decorator to DAG class [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #35360:
URL: https://github.com/apache/airflow/pull/35360#issuecomment-1789695369
Better luck next time
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org