[GitHub] [airflow] dstandish commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
dstandish commented on a change in pull request #19726: URL: https://github.com/apache/airflow/pull/19726#discussion_r763526991 ## File path: airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py ## @@ -21,18 +21,16 @@ from kubernetes.client import ApiClient, models as k8s from airflow.exceptions import AirflowException -from airflow.providers.cncf.kubernetes.backcompat.pod import Port, Resources -from airflow.providers.cncf.kubernetes.backcompat.pod_runtime_info_env import PodRuntimeInfoEnv -def _convert_kube_model_object(obj, old_class, new_class): +def _convert_kube_model_object(obj, new_class): convert_op = getattr(obj, "to_k8s_client_obj", None) if callable(convert_op): return obj.to_k8s_client_obj() Review comment: > PS: Isn't it implied by definition of "Deprecation"? Removal is implied by the word deprecation, yes. However, KPO is not making any deprecation warning (which is my concern) yet the KPO _will eventually_ remove this logic. I think 2 deprecations warnings is the lesser of 2 evils here (one for "backcompat is gonna be removed" the other for "we aren't gonna convert forever"), but not a hill to die on and i don't stand in the way. Further, the logic [here](https://github.com/apache/airflow/pull/19726/files#diff-fe570c8d3e660e4fca5e3d1f4be8f3c075d30c37d41d2f66f90d5bfbde964f33R76) does not even assume a backcompat class -- it is logic to accept kwargs from a dict. So in that case the user won't have a deprecation warning at all. Again no dying on hills for me here either and i defer to @jedcunningham -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
dstandish commented on a change in pull request #19726: URL: https://github.com/apache/airflow/pull/19726#discussion_r763473491 ## File path: airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py ## @@ -21,18 +21,16 @@ from kubernetes.client import ApiClient, models as k8s from airflow.exceptions import AirflowException -from airflow.providers.cncf.kubernetes.backcompat.pod import Port, Resources -from airflow.providers.cncf.kubernetes.backcompat.pod_runtime_info_env import PodRuntimeInfoEnv -def _convert_kube_model_object(obj, old_class, new_class): +def _convert_kube_model_object(obj, new_class): convert_op = getattr(obj, "to_k8s_client_obj", None) if callable(convert_op): return obj.to_k8s_client_obj() Review comment: @jedcunningham, do you think we should add deprecation warning here (i.e. before calling `to_k8s_client_obj`) in addition to [here](https://github.com/apache/airflow/pull/19726/files#diff-fe570c8d3e660e4fca5e3d1f4be8f3c075d30c37d41d2f66f90d5bfbde964f33R76)? Users who import the backcompat modules will get warnings when they do so; but maybe we should _also_ signal here that this logic to convert will be removed. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
dstandish commented on a change in pull request #19726: URL: https://github.com/apache/airflow/pull/19726#discussion_r763473491 ## File path: airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py ## @@ -21,18 +21,16 @@ from kubernetes.client import ApiClient, models as k8s from airflow.exceptions import AirflowException -from airflow.providers.cncf.kubernetes.backcompat.pod import Port, Resources -from airflow.providers.cncf.kubernetes.backcompat.pod_runtime_info_env import PodRuntimeInfoEnv -def _convert_kube_model_object(obj, old_class, new_class): +def _convert_kube_model_object(obj, new_class): convert_op = getattr(obj, "to_k8s_client_obj", None) if callable(convert_op): return obj.to_k8s_client_obj() Review comment: @jedcunningham, do you think we should add deprecation warning here in addition to [here](https://github.com/apache/airflow/pull/19726/files#diff-fe570c8d3e660e4fca5e3d1f4be8f3c075d30c37d41d2f66f90d5bfbde964f33R76)? Users who import the backcompat modules will get warnings when they do so; but maybe we should _also_ signal here that this logic to convert will be removed. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
dstandish commented on a change in pull request #19726: URL: https://github.com/apache/airflow/pull/19726#discussion_r763462578 ## File path: airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py ## @@ -21,18 +21,16 @@ from kubernetes.client import ApiClient, models as k8s from airflow.exceptions import AirflowException -from airflow.providers.cncf.kubernetes.backcompat.pod import Port, Resources -from airflow.providers.cncf.kubernetes.backcompat.pod_runtime_info_env import PodRuntimeInfoEnv -def _convert_kube_model_object(obj, old_class, new_class): +def _convert_kube_model_object(obj, old_class_name, new_class): Review comment: Your comment reminds me that by the time we get to this bit of code (e.g. if an airflow backcompat class is used) then the user will _already_ have imported the backcompat module, and the warning presumably would have already been emitted. So no need, I suppose, to add another warning here as I have suggested in option 1. That said I would still just get rid of the `old_class_name` param since it isn't doing anything useful. We can just say we expected class `new_class` or the equivalent airflow backcompat class.. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
dstandish commented on a change in pull request #19726: URL: https://github.com/apache/airflow/pull/19726#discussion_r763429100 ## File path: airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py ## @@ -21,18 +21,16 @@ from kubernetes.client import ApiClient, models as k8s from airflow.exceptions import AirflowException -from airflow.providers.cncf.kubernetes.backcompat.pod import Port, Resources -from airflow.providers.cncf.kubernetes.backcompat.pod_runtime_info_env import PodRuntimeInfoEnv -def _convert_kube_model_object(obj, old_class, new_class): +def _convert_kube_model_object(obj, old_class_name, new_class): Review comment: OK I think I get it now. We used to use our own classes representing k8s objects. Now we use the "official" ones. For _our_ versions, we have a conversion method `to_k8s_client_obj` And we want a deprecation warning raised when and only when we actually have to convert the object. Ok so here's my suggestion. I can think of three approaches (i like the earlier ones better than the later ones) 1. add depreciation just before line 29 where `obj. to_k8s_client_obj()` is called. In this case we know we are converting and can appropriately raise the warning. or... 2. Add a deprecation warning to every `to_k8s_client_obj` so that whenever it is called, there's a warning. or 3. add a base K8sObject class with a `to_k8s_client_obj` that emits the warning and ensure every backcompat airflow object inherits from it and calls `super(). to_k8s_client_obj` With either of these approaches, we don't need to import the backcompat modules. We don't really need to know (or specify) the specific backcompat class expected in `_convert_kube_model_object` -- we can just explain we expected the k8s object X or a backcompat class appropriately convertible to X. this would be my suggestion. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
dstandish commented on a change in pull request #19726: URL: https://github.com/apache/airflow/pull/19726#discussion_r763429100 ## File path: airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py ## @@ -21,18 +21,16 @@ from kubernetes.client import ApiClient, models as k8s from airflow.exceptions import AirflowException -from airflow.providers.cncf.kubernetes.backcompat.pod import Port, Resources -from airflow.providers.cncf.kubernetes.backcompat.pod_runtime_info_env import PodRuntimeInfoEnv -def _convert_kube_model_object(obj, old_class, new_class): +def _convert_kube_model_object(obj, old_class_name, new_class): Review comment: OK I think I get it now. We used to use our own classes representing k8s objects. Now we use the "official" ones. For _our_ versions, we have a conversion method `to_k8s_client_obj` And we want a deprecation warning raised when and only when we actually have to convert the object. Ok so here's my suggestion. I can think of two approaches. 1. add depreciation just before line 29 where `obj. to_k8s_client_obj()` is called. In this case we know we are converting and can appropriately raise the warning. or... 2. Add a deprecation warning to every `to_k8s_client_obj` so that whenever it is called, there's a warning. or 3. add a base K8sObject class with a `to_k8s_client_obj` that emits the warning and ensure every backcompat airflow object inherits from it and calls `super(). to_k8s_client_obj` With either of these approaches, we don't need to import the backcompat modules. We don't really need to know (or specify) the specific backcompat class expected in `_convert_kube_model_object` -- we can just explain we expected the k8s object X or a backcompat class appropriately convertible to X. this would be my suggestion. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
dstandish commented on a change in pull request #19726: URL: https://github.com/apache/airflow/pull/19726#discussion_r763421227 ## File path: airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py ## @@ -21,18 +21,16 @@ from kubernetes.client import ApiClient, models as k8s from airflow.exceptions import AirflowException -from airflow.providers.cncf.kubernetes.backcompat.pod import Port, Resources -from airflow.providers.cncf.kubernetes.backcompat.pod_runtime_info_env import PodRuntimeInfoEnv -def _convert_kube_model_object(obj, old_class, new_class): +def _convert_kube_model_object(obj, old_class_name, new_class): Review comment: > Of course I could remove mention of old class altogether so that user is not even considering it i'm not saying for certain that it should be removed, just taking a look and trying to understand. i'll have a closer look at the code but in the meantime don't consider my question a blocker. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
dstandish commented on a change in pull request #19726: URL: https://github.com/apache/airflow/pull/19726#discussion_r763421227 ## File path: airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py ## @@ -21,18 +21,16 @@ from kubernetes.client import ApiClient, models as k8s from airflow.exceptions import AirflowException -from airflow.providers.cncf.kubernetes.backcompat.pod import Port, Resources -from airflow.providers.cncf.kubernetes.backcompat.pod_runtime_info_env import PodRuntimeInfoEnv -def _convert_kube_model_object(obj, old_class, new_class): +def _convert_kube_model_object(obj, old_class_name, new_class): Review comment: > Of course I could remove mention of old class altogether so that user is not even considering it i'm not saying for certain that it should be removed, just taking a look and trying to understand. i'll have a closer look at the code but don't consider my question in any way to be a blocker. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
dstandish commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r763415973
##
File path:
airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py
##
@@ -21,18 +21,16 @@
from kubernetes.client import ApiClient, models as k8s
from airflow.exceptions import AirflowException
-from airflow.providers.cncf.kubernetes.backcompat.pod import Port, Resources
-from airflow.providers.cncf.kubernetes.backcompat.pod_runtime_info_env import
PodRuntimeInfoEnv
-def _convert_kube_model_object(obj, old_class, new_class):
+def _convert_kube_model_object(obj, old_class_name, new_class):
Review comment:
it seems odd that the exception is, in part,`expected {old class name}`,
when we do not appear to _check_ anywhere that it is `old_class_name` is
that actually being checked? if not why are we indicating that in the error?
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
dstandish commented on a change in pull request #19726: URL: https://github.com/apache/airflow/pull/19726#discussion_r763410468 ## File path: airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py ## @@ -21,18 +21,16 @@ from kubernetes.client import ApiClient, models as k8s from airflow.exceptions import AirflowException -from airflow.providers.cncf.kubernetes.backcompat.pod import Port, Resources -from airflow.providers.cncf.kubernetes.backcompat.pod_runtime_info_env import PodRuntimeInfoEnv -def _convert_kube_model_object(obj, old_class, new_class): +def _convert_kube_model_object(obj, old_class_name, new_class): Review comment: i'm confused why parameter `old_class_name` is here at all. do you know? it does not appear it has any effect on the function's behavior could we instead just remove this param? -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
