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 2021/12/30 03:42:27 UTC
[GitHub] [airflow] dstandish opened a new pull request #20575: Delete pods by default in KubernetesPodOperator
dstandish opened a new pull request #20575:
URL: https://github.com/apache/airflow/pull/20575
We change the default for `is_delete_operator_pod` to `True`. For subclasses `GKEStartPodOperator` and `EksPodOperator` we do not _yet_ change the default since we may not want to do a major release in those providers. Instead we identify when the parameter is not set and emit a deprecation warning to notify users of the impending 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] potiuk commented on a change in pull request #20575: Delete pods by default in KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20575:
URL: https://github.com/apache/airflow/pull/20575#discussion_r776807677
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -18,12 +18,13 @@
"""This module contains Amazon EKS operators."""
import warnings
from time import sleep
-from typing import Dict, Iterable, List, Optional
+from typing import Dict, Iterable, List, Optional, Union
from airflow import AirflowException
from airflow.models import BaseOperator
from airflow.providers.amazon.aws.hooks.eks import ClusterStates, EksHook, FargateProfileStates
from airflow.providers.cncf.kubernetes.operators.kubernetes_pod import KubernetesPodOperator
+from airflow.utils.types import NOTSET, ArgNotSet
Review comment:
Ah yeah. That's price to pay for provider's flexibility, I am afraid.
--
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 #20575: Delete pods by default in KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20575:
URL: https://github.com/apache/airflow/pull/20575#issuecomment-1003195684
Some static checks fail.
--
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 change in pull request #20575: Delete pods by default in KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20575:
URL: https://github.com/apache/airflow/pull/20575#discussion_r776809144
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -18,12 +18,13 @@
"""This module contains Amazon EKS operators."""
import warnings
from time import sleep
-from typing import Dict, Iterable, List, Optional
+from typing import Dict, Iterable, List, Optional, Union
from airflow import AirflowException
from airflow.models import BaseOperator
from airflow.providers.amazon.aws.hooks.eks import ClusterStates, EksHook, FargateProfileStates
from airflow.providers.cncf.kubernetes.operators.kubernetes_pod import KubernetesPodOperator
+from airflow.utils.types import NOTSET, ArgNotSet
Review comment:
And the provider import tests helps with making sure it's not missed :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] potiuk commented on a change in pull request #20575: Delete pods by default in KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20575:
URL: https://github.com/apache/airflow/pull/20575#discussion_r776753146
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -18,12 +18,13 @@
"""This module contains Amazon EKS operators."""
import warnings
from time import sleep
-from typing import Dict, Iterable, List, Optional
+from typing import Dict, Iterable, List, Optional, Union
from airflow import AirflowException
from airflow.models import BaseOperator
from airflow.providers.amazon.aws.hooks.eks import ClusterStates, EksHook, FargateProfileStates
from airflow.providers.cncf.kubernetes.operators.kubernetes_pod import KubernetesPodOperator
+from airflow.utils.types import NOTSET, ArgNotSet
Review comment:
Currently all providers are tested with importing everything in 2.1 - so this will basically fail the provider test when rebased (and conflicts resolved).
--
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] dstandish commented on pull request #20575: Delete pods by default in KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #20575:
URL: https://github.com/apache/airflow/pull/20575#issuecomment-1003185027
ready for [re]perusal
--
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 change in pull request #20575: Delete pods by default in KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20575:
URL: https://github.com/apache/airflow/pull/20575#discussion_r776752577
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -18,12 +18,13 @@
"""This module contains Amazon EKS operators."""
import warnings
from time import sleep
-from typing import Dict, Iterable, List, Optional
+from typing import Dict, Iterable, List, Optional, Union
from airflow import AirflowException
from airflow.models import BaseOperator
from airflow.providers.amazon.aws.hooks.eks import ClusterStates, EksHook, FargateProfileStates
from airflow.providers.cncf.kubernetes.operators.kubernetes_pod import KubernetesPodOperator
+from airflow.utils.types import NOTSET, ArgNotSet
Review comment:
Yes
--
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] dstandish commented on a change in pull request #20575: Delete pods by default in KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20575:
URL: https://github.com/apache/airflow/pull/20575#discussion_r776869164
##########
File path: airflow/providers/cncf/kubernetes/CHANGELOG.rst
##########
@@ -26,6 +26,8 @@ Breaking changes
~~~~~~~~~~~~~~~~
* ``Simplify KubernetesPodOperator (#19572)``
+* Parameter ``is_delete_operator_pod`` default is changed to ``True``
Review comment:
done, ptal
--
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 change in pull request #20575: Delete pods by default in KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20575:
URL: https://github.com/apache/airflow/pull/20575#discussion_r776753146
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -18,12 +18,13 @@
"""This module contains Amazon EKS operators."""
import warnings
from time import sleep
-from typing import Dict, Iterable, List, Optional
+from typing import Dict, Iterable, List, Optional, Union
from airflow import AirflowException
from airflow.models import BaseOperator
from airflow.providers.amazon.aws.hooks.eks import ClusterStates, EksHook, FargateProfileStates
from airflow.providers.cncf.kubernetes.operators.kubernetes_pod import KubernetesPodOperator
+from airflow.utils.types import NOTSET, ArgNotSet
Review comment:
Currently all providers are tested with importing everything in 2.1 - so this will basically fail the provider test when rebased.
--
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 #20575: Delete pods by default in KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20575:
URL: https://github.com/apache/airflow/pull/20575#issuecomment-1003313796
> I decided to resolved this by setting explicit value for `is_delete_operator_pod=True` in the example dag, to be explicit and update it to be the new default (previously the behavior would have been False).
I really like how our tests are catching those cases. Because we actually found this way that our example docs were giving bad advice to our users (after the change) and we also improved the advice.
--
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] github-actions[bot] commented on pull request #20575: Delete pods by default in KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20575:
URL: https://github.com/apache/airflow/pull/20575#issuecomment-1003195709
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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] kaxil commented on a change in pull request #20575: Delete pods by default in KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #20575:
URL: https://github.com/apache/airflow/pull/20575#discussion_r776735254
##########
File path: airflow/providers/cncf/kubernetes/CHANGELOG.rst
##########
@@ -26,6 +26,8 @@ Breaking changes
~~~~~~~~~~~~~~~~
* ``Simplify KubernetesPodOperator (#19572)``
+* Parameter ``is_delete_operator_pod`` default is changed to ``True``
Review comment:
Can you add the rationale on why we are making this change for users to understand it better
--
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] dstandish commented on a change in pull request #20575: Delete pods by default in KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20575:
URL: https://github.com/apache/airflow/pull/20575#discussion_r776869078
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -18,12 +18,13 @@
"""This module contains Amazon EKS operators."""
import warnings
from time import sleep
-from typing import Dict, Iterable, List, Optional
+from typing import Dict, Iterable, List, Optional, Union
from airflow import AirflowException
from airflow.models import BaseOperator
from airflow.providers.amazon.aws.hooks.eks import ClusterStates, EksHook, FargateProfileStates
from airflow.providers.cncf.kubernetes.operators.kubernetes_pod import KubernetesPodOperator
+from airflow.utils.types import NOTSET, ArgNotSet
Review comment:
OK i have replaced NOTSET with None
None works fine in this case because for KPO None is not allowed
--
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] dstandish commented on pull request #20575: Delete pods by default in KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #20575:
URL: https://github.com/apache/airflow/pull/20575#issuecomment-1003243270
> Build and test provider packages wheel failed with the following:
This was due to an example dag which instantiated the GKE subclass without specifying a default for `is_delete_operator_pod` (which we're warning about now since the default has changed in KPO).
I decided to resolved this by setting explicit value for `is_delete_operator_pod=True` in the example dag, to be explicit and update it to be the new default (previously the behavior would have been False).
in this commit: https://github.com/apache/airflow/pull/20575/commits/8c6a574beda993795511d82a195f13c42d3f422c
--
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 #20575: Delete pods by default in KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #20575:
URL: https://github.com/apache/airflow/pull/20575
--
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] kaxil commented on a change in pull request #20575: Delete pods by default in KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #20575:
URL: https://github.com/apache/airflow/pull/20575#discussion_r776808407
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -18,12 +18,13 @@
"""This module contains Amazon EKS operators."""
import warnings
from time import sleep
-from typing import Dict, Iterable, List, Optional
+from typing import Dict, Iterable, List, Optional, Union
from airflow import AirflowException
from airflow.models import BaseOperator
from airflow.providers.amazon.aws.hooks.eks import ClusterStates, EksHook, FargateProfileStates
from airflow.providers.cncf.kubernetes.operators.kubernetes_pod import KubernetesPodOperator
+from airflow.utils.types import NOTSET, ArgNotSet
Review comment:
While they are in the same repo, you should really think of it like a Terraform provider which is in a separate repo and a separate source code, just depends on the core.
And especially for **utilities** not rely on the core package
--
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] dstandish commented on a change in pull request #20575: Delete pods by default in KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20575:
URL: https://github.com/apache/airflow/pull/20575#discussion_r776807136
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -18,12 +18,13 @@
"""This module contains Amazon EKS operators."""
import warnings
from time import sleep
-from typing import Dict, Iterable, List, Optional
+from typing import Dict, Iterable, List, Optional, Union
from airflow import AirflowException
from airflow.models import BaseOperator
from airflow.providers.amazon.aws.hooks.eks import ClusterStates, EksHook, FargateProfileStates
from airflow.providers.cncf.kubernetes.operators.kubernetes_pod import KubernetesPodOperator
+from airflow.utils.types import NOTSET, ArgNotSet
Review comment:
dangit, min version strikes again!
--
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] josh-fell commented on a change in pull request #20575: Delete pods by default in KubernetesPodOperator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on a change in pull request #20575:
URL: https://github.com/apache/airflow/pull/20575#discussion_r776750389
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -18,12 +18,13 @@
"""This module contains Amazon EKS operators."""
import warnings
from time import sleep
-from typing import Dict, Iterable, List, Optional
+from typing import Dict, Iterable, List, Optional, Union
from airflow import AirflowException
from airflow.models import BaseOperator
from airflow.providers.amazon.aws.hooks.eks import ClusterStates, EksHook, FargateProfileStates
from airflow.providers.cncf.kubernetes.operators.kubernetes_pod import KubernetesPodOperator
+from airflow.utils.types import NOTSET, ArgNotSet
Review comment:
Would using these in providers force the minimum Airflow version required to be whatever version `airflow.utils.types.NOTSET/ArgNotSet` are released with?
--
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