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 2020/10/15 18:39:20 UTC
[GitHub] [airflow] mjpieters opened a new pull request #11558: Guard against kubernetes not being installed
mjpieters opened a new pull request #11558:
URL: https://github.com/apache/airflow/pull/11558
If the `kubernetes.client` import fails, then `airflow.kubernetes.pod_generator` also can't be imported, and there won't be attributes on `k8s` to use in `isinstance()` calls.
Instead of setting `k8s` to `None`, use an explicit flag so later code can disable kubernetes-specific branches explicitly.
closes: #11556
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #11558: Guard against kubernetes not being installed
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #11558:
URL: https://github.com/apache/airflow/pull/11558#issuecomment-710112685
Awesome work, congrats on your first merged pull request!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mjpieters commented on a change in pull request #11558: Guard against kubernetes not being installed
Posted by GitBox <gi...@apache.org>.
mjpieters commented on a change in pull request #11558:
URL: https://github.com/apache/airflow/pull/11558#discussion_r506763359
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -187,12 +190,12 @@ def _serialize(cls, var: Any) -> Any: # Unfortunately there is no support for r
{str(k): cls._serialize(v) for k, v in var.items()},
type_=DAT.DICT
)
- elif isinstance(var, k8s.V1Pod):
+ elif HAS_KUBERNETES and isinstance(var, k8s.V1Pod):
json_pod = PodGenerator.serialize_pod(var)
return cls._encode(json_pod, type_=DAT.POD)
elif isinstance(var, list):
return [cls._serialize(v) for v in var]
- elif isinstance(var, k8s.V1Pod):
+ elif HAS_KUBERNETES and isinstance(var, k8s.V1Pod):
Review comment:
This block is redundant actually; no need to test for k8s.V1Pod _twice_.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #11558: Guard against kubernetes not being installed
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11558:
URL: https://github.com/apache/airflow/pull/11558#issuecomment-710048323
[The Workflow run](https://github.com/apache/airflow/actions/runs/310743426) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11558: Guard against kubernetes not being installed
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11558:
URL: https://github.com/apache/airflow/pull/11558#discussion_r506764891
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -187,12 +190,12 @@ def _serialize(cls, var: Any) -> Any: # Unfortunately there is no support for r
{str(k): cls._serialize(v) for k, v in var.items()},
type_=DAT.DICT
)
- elif isinstance(var, k8s.V1Pod):
+ elif HAS_KUBERNETES and isinstance(var, k8s.V1Pod):
json_pod = PodGenerator.serialize_pod(var)
return cls._encode(json_pod, type_=DAT.POD)
elif isinstance(var, list):
return [cls._serialize(v) for v in var]
- elif isinstance(var, k8s.V1Pod):
+ elif HAS_KUBERNETES and isinstance(var, k8s.V1Pod):
Review comment:
PR: https://github.com/apache/airflow/pull/11602
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb merged pull request #11558: Guard against kubernetes not being installed
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #11558:
URL: https://github.com/apache/airflow/pull/11558
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11558: Guard against kubernetes not being installed
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11558:
URL: https://github.com/apache/airflow/pull/11558#discussion_r506163065
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -187,12 +190,12 @@ def _serialize(cls, var: Any) -> Any: # Unfortunately there is no support for r
{str(k): cls._serialize(v) for k, v in var.items()},
type_=DAT.DICT
)
- elif isinstance(var, k8s.V1Pod):
+ elif has_kubernetes and isinstance(var, k8s.V1Pod):
Review comment:
```suggestion
elif HAS_KUBERNETES and isinstance(var, k8s.V1Pod):
```
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -187,12 +190,12 @@ def _serialize(cls, var: Any) -> Any: # Unfortunately there is no support for r
{str(k): cls._serialize(v) for k, v in var.items()},
type_=DAT.DICT
)
- elif isinstance(var, k8s.V1Pod):
+ elif has_kubernetes and isinstance(var, k8s.V1Pod):
json_pod = PodGenerator.serialize_pod(var)
return cls._encode(json_pod, type_=DAT.POD)
elif isinstance(var, list):
return [cls._serialize(v) for v in var]
- elif isinstance(var, k8s.V1Pod):
+ elif has_kubernetes and isinstance(var, k8s.V1Pod):
Review comment:
```suggestion
elif HAS_KUBERNETES and isinstance(var, k8s.V1Pod):
```
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -256,7 +259,7 @@ def _deserialize(cls, encoded_var: Any) -> Any: # pylint: disable=too-many-retu
return SerializedBaseOperator.deserialize_operator(var)
elif type_ == DAT.DATETIME:
return pendulum.from_timestamp(var)
- elif type_ == DAT.POD:
+ elif has_kubernetes and type_ == DAT.POD:
Review comment:
Though I also wonder if it should be an error to try and deserialise a pod without the kube modules installed?
So something like:
```suggestion
elif type_ == DAT.POD:
if not HAS_KUBERNETES:
raise RuntimeError("Cannot deserialize POD objects without kubernetes libraries installed!")
```
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -256,7 +259,7 @@ def _deserialize(cls, encoded_var: Any) -> Any: # pylint: disable=too-many-retu
return SerializedBaseOperator.deserialize_operator(var)
elif type_ == DAT.DATETIME:
return pendulum.from_timestamp(var)
- elif type_ == DAT.POD:
+ elif has_kubernetes and type_ == DAT.POD:
Review comment:
```suggestion
elif HAS_KUBERNETES and type_ == DAT.POD:
```
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -46,6 +39,16 @@
from airflow.utils.module_loading import import_string
from airflow.utils.task_group import TaskGroup
+try:
+ # isort: off
+ from kubernetes.client import models as k8s
+ from airflow.kubernetes.pod_generator import PodGenerator
+ # isort: on
+ has_kubernetes = True
+except ImportError:
+ has_kubernetes = False
Review comment:
```suggestion
HAS_KUBERNETES = True
except ImportError:
HAS_KUBERNETES = False
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #11558: Guard against kubernetes not being installed
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11558:
URL: https://github.com/apache/airflow/pull/11558#issuecomment-709522387
Would you mind adding a unit test for it? I think it would be possible to mock import https://stackoverflow.com/questions/8658043/how-to-mock-an-import
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org