You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/08/18 15:23:24 UTC
[GitHub] [airflow] eladkal opened a new pull request, #25799: [WIP] Add Airflow specific warning classes
eladkal opened a new pull request, #25799:
URL: https://github.com/apache/airflow/pull/25799
Adding `RemoveInAirflow3DeprecationWarning` and `AirflowProviderDeprecationWarning`
`RemoveInAirflow3DeprecationWarning` - modifying all occurrences of `DeprecationWarning` in core to the new class.
`AirflowProviderDeprecationWarning` - only adding the class. We can not yet replace warnings in provider. This will be useful when minimum provider version would be on Airflow 2.4 (or the release of this PR)
Closes: https://github.com/apache/airflow/issues/22356
**Todo:**
[ ] Verify no missing directories
[ ] fix tests of deprecation warnings
[ ] We have some occurrences of `PendingDeprecationWarning` in the code base but I'm not sure if we have that distinction? Should we also have `RemoveInAirflow3PendingDeprecationWarning` ?
<!--
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/
-->
---
**^ 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
[GitHub] [airflow] blag commented on a diff in pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
blag commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r951991494
##########
airflow/exceptions.py:
##########
@@ -338,3 +338,17 @@ class TaskDeferralError(AirflowException):
class PodReconciliationError(AirflowException):
"""Raised when an error is encountered while trying to merge pod configs."""
+
+
+class RemoveInAirflow3DeprecationWarning(DeprecationWarning):
Review Comment:
```suggestion
class RemovedInAirflow3DeprecationWarning(DeprecationWarning):
```
Nitpick: Make this a past-tense description of the warning, instead of a command. This makes it consistent with its docstring and other exceptions in `airflow.exception`.
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r949448209
##########
airflow/configuration.py:
##########
@@ -50,7 +50,7 @@
# show Airflow's deprecation warnings
if not sys.warnoptions:
warnings.filterwarnings(action='default', category=DeprecationWarning, module='airflow')
- warnings.filterwarnings(action='default', category=PendingDeprecationWarning, module='airflow')
+ warnings.filterwarnings(action='default', category=RemoveInAirflow3DeprecationWarning, module='airflow')
Review Comment:
I’d probably just keep this as-is. Also note that adding `RemoveInAirflow3DeprecationWarning` here does not actually do anything, since it is a subclass of `DeprecationWarning` and already enabled by line 52.
--
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
[GitHub] [airflow] blag commented on a diff in pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
blag commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r951991494
##########
airflow/exceptions.py:
##########
@@ -338,3 +338,17 @@ class TaskDeferralError(AirflowException):
class PodReconciliationError(AirflowException):
"""Raised when an error is encountered while trying to merge pod configs."""
+
+
+class RemoveInAirflow3DeprecationWarning(DeprecationWarning):
Review Comment:
```suggestion
class RemovedInAirflow3Warning(DeprecationWarning):
```
Nitpick: Make this a past-tense description of the warning, instead of a command. This makes it consistent with its docstring and other exceptions in `airflow.exception`.
--
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
[GitHub] [airflow] blag commented on a diff in pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
blag commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r952030122
##########
airflow/exceptions.py:
##########
@@ -338,3 +338,17 @@ class TaskDeferralError(AirflowException):
class PodReconciliationError(AirflowException):
"""Raised when an error is encountered while trying to merge pod configs."""
+
+
+class RemoveInAirflow3DeprecationWarning(DeprecationWarning):
Review Comment:
`PendingDeprecationWarning`s and `DeprecationWarning`s are [filtered out](https://docs.python.org/3/library/warnings.html#default-warning-filter) by default. ~I think subclassing from `FutureWarning` would make them more visible to Airflow DAG authors.~
--
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
[GitHub] [airflow] potiuk merged pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #25799:
URL: https://github.com/apache/airflow/pull/25799
--
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
[GitHub] [airflow] zachliu commented on a diff in pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
zachliu commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r960003587
##########
airflow/exceptions.py:
##########
@@ -338,3 +338,17 @@ class TaskDeferralError(AirflowException):
class PodReconciliationError(AirflowException):
"""Raised when an error is encountered while trying to merge pod configs."""
+
+
+class RemoveInAirflow3DeprecationWarning(DeprecationWarning):
Review Comment:
sorry to comment here, but i'm obsessed with DeprecationWarning recently because i couldn't find mine :joy: :
https://apache-airflow.slack.com/archives/CSS36QQS1/p1661977783108899?thread_ts=1661974104.498239&cid=CSS36QQS1
--
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
[GitHub] [airflow] eladkal commented on pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
eladkal commented on PR #25799:
URL: https://github.com/apache/airflow/pull/25799#issuecomment-1223730828
>Should we add a pre-commit to check automatically if there are no "standard" warnings in Airflow code? I think it should be as easy as using pygrep pre-commit (we already use it in a few places).
Added
--
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
[GitHub] [airflow] eladkal commented on a diff in pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r949451143
##########
airflow/configuration.py:
##########
@@ -50,7 +50,7 @@
# show Airflow's deprecation warnings
if not sys.warnoptions:
warnings.filterwarnings(action='default', category=DeprecationWarning, module='airflow')
- warnings.filterwarnings(action='default', category=PendingDeprecationWarning, module='airflow')
+ warnings.filterwarnings(action='default', category=RemoveInAirflow3DeprecationWarning, module='airflow')
Review Comment:
Yes I was wondering about:
```diff
-warnings.filterwarnings(action='default', category=DeprecationWarning, module='airflow')
-warnings.filterwarnings(action='default', category=PendingDeprecationWarning, module='airflow')
+warnings.filterwarnings(action='default', category=RemoveInAirflow3DeprecationWarning, module='airflow')
```
--
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
[GitHub] [airflow] potiuk commented on pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25799:
URL: https://github.com/apache/airflow/pull/25799#issuecomment-1223340407
Comment: We COULD potentially do the same approach as with common.sql and introduce 'apache-airflow-providers-common-provider` and add the AirflowProviderDeprecationWarning.
If the package is very small, we can keep it as a way to add functionality that is common to all providers
--
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
[GitHub] [airflow] eladkal commented on a diff in pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r950798754
##########
airflow/exceptions.py:
##########
@@ -338,3 +338,17 @@ class TaskDeferralError(AirflowException):
class PodReconciliationError(AirflowException):
"""Raised when an error is encountered while trying to merge pod configs."""
+
+
+class RemoveInAirflow3DeprecationWarning(DeprecationWarning):
Review Comment:
We consider Airflow users as Python developers not end users don't we?
--
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
[GitHub] [airflow] eladkal commented on a diff in pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r949451143
##########
airflow/configuration.py:
##########
@@ -50,7 +50,7 @@
# show Airflow's deprecation warnings
if not sys.warnoptions:
warnings.filterwarnings(action='default', category=DeprecationWarning, module='airflow')
- warnings.filterwarnings(action='default', category=PendingDeprecationWarning, module='airflow')
+ warnings.filterwarnings(action='default', category=RemoveInAirflow3DeprecationWarning, module='airflow')
Review Comment:
Yes I was wondering about:
```diff
-warnings.filterwarnings(action='default', category=DeprecationWarning, module='airflow')
-warnings.filterwarnings(action='default', category=PendingDeprecationWarning, module='airflow')
+warnings.filterwarnings(action='default', category=RemoveInAirflow3DeprecationWarning, module='airflow')
```
our goal here is not to have general `DeprecationWarning` in the code base
--
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
[GitHub] [airflow] eladkal commented on pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
eladkal commented on PR #25799:
URL: https://github.com/apache/airflow/pull/25799#issuecomment-1223562315
> And we could replace the provider's warnings even now.
We can but to be honest I think it has very little value. With the new provider governance module there is no reason to keep deprecations for so long. We can remove them really fast even in a follow up release with is 1 month window and cherry pick features to earlier releases.
Also, after 2.4 is minimum Airflow version for providers you probably won't need `apache-airflow-providers-common-provider` to store the deprecation class as you will just import it from core so I don't think it's worth the effort
--
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
[GitHub] [airflow] eladkal commented on a diff in pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r952161292
##########
airflow/exceptions.py:
##########
@@ -338,3 +338,17 @@ class TaskDeferralError(AirflowException):
class PodReconciliationError(AirflowException):
"""Raised when an error is encountered while trying to merge pod configs."""
+
+
+class RemoveInAirflow3DeprecationWarning(DeprecationWarning):
Review Comment:
https://github.com/apache/airflow/pull/25799/commits/583d6c1b888c4a3fb33543e80f842a4103c5aa5c
--
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
[GitHub] [airflow] blag commented on a diff in pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
blag commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r952032327
##########
airflow/exceptions.py:
##########
@@ -338,3 +338,17 @@ class TaskDeferralError(AirflowException):
class PodReconciliationError(AirflowException):
"""Raised when an error is encountered while trying to merge pod configs."""
+
+
+class RemoveInAirflow3DeprecationWarning(DeprecationWarning):
Review Comment:
However, we may not want this to be immediately visible to DAG authors, because at this point we [should still assume that we will only have 2.x releases](https://apache-airflow.slack.com/archives/CCPRP7943/p1661211652517059).
Subclassing from DeprecationWarning (or PendingDeprecationWarning) might actually be the right call here until we have concrete plans for Airflow 3.0. [PEP-0387](https://peps.python.org/pep-0387/#making-incompatible-changes):
> If an API is being removed, simply warn whenever it is entered. DeprecationWarning is the usual warning category to use, but **PendingDeprecationWarning may be used in special cases where the old and new versions of the API will coexist for many releases [[2]](https://peps.python.org/pep-0387/#warnings)**.
(emphasis mine)
--
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
[GitHub] [airflow] potiuk commented on pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25799:
URL: https://github.com/apache/airflow/pull/25799#issuecomment-1223343838
Should we add a pre-commit to check automatically if there are no "standard" warnings in Airflow code? I think it should be as easy as using `pygrep` pre-commit (we already use it in a few places).
--
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
[GitHub] [airflow] eladkal commented on a diff in pull request #25799: [WIP] Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r949428164
##########
airflow/configuration.py:
##########
@@ -50,7 +50,7 @@
# show Airflow's deprecation warnings
if not sys.warnoptions:
warnings.filterwarnings(action='default', category=DeprecationWarning, module='airflow')
- warnings.filterwarnings(action='default', category=PendingDeprecationWarning, module='airflow')
+ warnings.filterwarnings(action='default', category=RemoveInAirflow3DeprecationWarning, module='airflow')
Review Comment:
I'm not sure if we should leave `PendingDeprecationWarning` as is and just add `RemoveInAirflow3DeprecationWarning` rather than replace?
--
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
[GitHub] [airflow] eladkal commented on a diff in pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r949451143
##########
airflow/configuration.py:
##########
@@ -50,7 +50,7 @@
# show Airflow's deprecation warnings
if not sys.warnoptions:
warnings.filterwarnings(action='default', category=DeprecationWarning, module='airflow')
- warnings.filterwarnings(action='default', category=PendingDeprecationWarning, module='airflow')
+ warnings.filterwarnings(action='default', category=RemoveInAirflow3DeprecationWarning, module='airflow')
Review Comment:
Yes I was wondering about:
```diff
-warnings.filterwarnings(action='default', category=DeprecationWarning, module='airflow')
-warnings.filterwarnings(action='default', category=PendingDeprecationWarning, module='airflow')
+warnings.filterwarnings(action='default', category=RemoveInAirflow3DeprecationWarning, module='airflow')
```
our goal here is not to have general `DeprecationWarning` in the code base (not that this enforce anything)
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r952035313
##########
airflow/exceptions.py:
##########
@@ -338,3 +338,17 @@ class TaskDeferralError(AirflowException):
class PodReconciliationError(AirflowException):
"""Raised when an error is encountered while trying to merge pod configs."""
+
+
+class RemoveInAirflow3DeprecationWarning(DeprecationWarning):
Review Comment:
We explicitly enable the Deprecation warnings coming from airflow following https://peps.python.org/pep-0565/#recommended-filter-settings-for-interactive-shells
See `airflow.configuration.py`:
```
if not sys.warnoptions:
warnings.filterwarnings(action='default', category=DeprecationWarning, module='airflow')
warnings.filterwarnings(action='default', category=PendingDeprecationWarning, module='airflow')
````
--
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
[GitHub] [airflow] blag commented on a diff in pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
blag commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r952056489
##########
airflow/exceptions.py:
##########
@@ -338,3 +338,17 @@ class TaskDeferralError(AirflowException):
class PodReconciliationError(AirflowException):
"""Raised when an error is encountered while trying to merge pod configs."""
+
+
+class RemoveInAirflow3DeprecationWarning(DeprecationWarning):
Review Comment:
I'm curious why #1285 "turned on" both [DeprecationWarning and PendingDeprecationWarning](https://github.com/apache/airflow/pull/1285/files#diff-2ab05df659c739ff335a407c4a6cddc5f177e9c402aca296991f53fa341f4dfeR20-R22).
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r953117664
##########
airflow/exceptions.py:
##########
@@ -338,3 +338,17 @@ class TaskDeferralError(AirflowException):
class PodReconciliationError(AirflowException):
"""Raised when an error is encountered while trying to merge pod configs."""
+
+
+class RemoveInAirflow3DeprecationWarning(DeprecationWarning):
Review Comment:
It was 6 years ago :) `5 Apr 2016` :). I am afraid no commiters who are active now were even around :D
--
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
[GitHub] [airflow] ashb commented on a diff in pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r950085661
##########
airflow/exceptions.py:
##########
@@ -338,3 +338,17 @@ class TaskDeferralError(AirflowException):
class PodReconciliationError(AirflowException):
"""Raised when an error is encountered while trying to merge pod configs."""
+
+
+class RemoveInAirflow3DeprecationWarning(DeprecationWarning):
Review Comment:
https://docs.python.org/3/library/warnings.html
DeprecationWarning | Base category for warnings about deprecated features when those warnings are intended for other Python developers (ignored by default, unless triggered by code in __main__).
-- | --
FutureWarning | Base category for warnings about deprecated features when those warnings are intended for end users of applications that are written in Python.
-- | --
PendingDeprecationWarning | Base category for warnings about features that will be deprecated in the future (ignored by default).
Should we make this baseclass be FutureWarning?
--
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
[GitHub] [airflow] ashb commented on a diff in pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r950085661
##########
airflow/exceptions.py:
##########
@@ -338,3 +338,17 @@ class TaskDeferralError(AirflowException):
class PodReconciliationError(AirflowException):
"""Raised when an error is encountered while trying to merge pod configs."""
+
+
+class RemoveInAirflow3DeprecationWarning(DeprecationWarning):
Review Comment:
https://docs.python.org/3/library/warnings.html
DeprecationWarning | Base category for warnings about deprecated features when those warnings are intended for other Python developers (ignored by default, unless triggered by code in __main__).
-- | --
FutureWarning | Base category for warnings about deprecated features when those warnings are intended for end users of applications that are written in Python.
-- | --
PendingDeprecationWarning | Base category for warnings about features that will be deprecated in the future (ignored by default).
-- | --
Should we make this baseclass be FutureWarning?
--
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
[GitHub] [airflow] eladkal commented on a diff in pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r949451143
##########
airflow/configuration.py:
##########
@@ -50,7 +50,7 @@
# show Airflow's deprecation warnings
if not sys.warnoptions:
warnings.filterwarnings(action='default', category=DeprecationWarning, module='airflow')
- warnings.filterwarnings(action='default', category=PendingDeprecationWarning, module='airflow')
+ warnings.filterwarnings(action='default', category=RemoveInAirflow3DeprecationWarning, module='airflow')
Review Comment:
Yes I was wondering about:
```diff
-warnings.filterwarnings(action='default', category=DeprecationWarning, module='airflow')
-warnings.filterwarnings(action='default', category=PendingDeprecationWarning, module='airflow')
+warnings.filterwarnings(action='default', category=RemoveInAirflow3DeprecationWarning, module='airflow')
```
**EDIT**: NVM we still have `DeprecationWarning` in `contrib` either way. I'll revert this change
--
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
[GitHub] [airflow] eladkal commented on a diff in pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r949454388
##########
airflow/configuration.py:
##########
@@ -50,7 +50,7 @@
# show Airflow's deprecation warnings
if not sys.warnoptions:
warnings.filterwarnings(action='default', category=DeprecationWarning, module='airflow')
- warnings.filterwarnings(action='default', category=PendingDeprecationWarning, module='airflow')
+ warnings.filterwarnings(action='default', category=RemoveInAirflow3DeprecationWarning, module='airflow')
Review Comment:
https://github.com/apache/airflow/pull/25799/commits/a54ee211784de62ba3b880b3f6f4b969e8880ac4
--
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
[GitHub] [airflow] blag commented on a diff in pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
blag commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r952030122
##########
airflow/exceptions.py:
##########
@@ -338,3 +338,17 @@ class TaskDeferralError(AirflowException):
class PodReconciliationError(AirflowException):
"""Raised when an error is encountered while trying to merge pod configs."""
+
+
+class RemoveInAirflow3DeprecationWarning(DeprecationWarning):
Review Comment:
`PendingDeprecationWarning`s and `DeprecationWarning`s are [filtered out](https://docs.python.org/3/library/warnings.html#default-warning-filter) by default. I think subclassing from `FutureWarning` would make them more visible to Airflow DAG authors.
--
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
[GitHub] [airflow] zachliu commented on a diff in pull request #25799: Add Airflow specific warning classes
Posted by GitBox <gi...@apache.org>.
zachliu commented on code in PR #25799:
URL: https://github.com/apache/airflow/pull/25799#discussion_r960003587
##########
airflow/exceptions.py:
##########
@@ -338,3 +338,17 @@ class TaskDeferralError(AirflowException):
class PodReconciliationError(AirflowException):
"""Raised when an error is encountered while trying to merge pod configs."""
+
+
+class RemoveInAirflow3DeprecationWarning(DeprecationWarning):
Review Comment:
sorry to comment here, but i'm obsessed with DeprecationWarning recently because i couldn't find mine :joy: :
https://apache-airflow.slack.com/archives/CSS36QQS1/p1661974104498239
--
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