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/16 16:28:02 UTC
[GitHub] [airflow] dimon222 opened a new pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
dimon222 opened a new pull request #19726:
URL: https://github.com/apache/airflow/pull/19726
What is this?
This is to resolve incomplete solution of this commit https://github.com/apache/airflow/commit/42e13e1a5a4c97a2085ddf96f7d93e7bf71949b8 (#17900)
The deprecation warning was previously shown always, but with above commit the scope was reduced. Unfortunately, its not doing what its intended to do - show this warning only when legacy Volume and VolumeMount classes are being used with KubernetesPodOperator. Instead, it now shows up always when you use Volume or VolumeMount of ANY implementation (old or new).
The reason for that is next:
1. The "conversion" methods are called on KPO always whenever there's any Volume/VolumeMount
https://github.com/apache/airflow/blob/1e570229533c4bbf5d3c901d5db21261fa4b1137/airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py#L246-L247
2. While those methods will convert only if necessary, inside of them we're doing imports of legacy class, which immediately shows message of deprecation upon import, rather than upon validation of underline class (so that we know if we have to import it in the first place...). In other words, we have a chicken-and-egg problem that might have been solved incorrectly.
https://github.com/apache/airflow/blob/1e570229533c4bbf5d3c901d5db21261fa4b1137/airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py#L55-L57
https://github.com/apache/airflow/blob/1e570229533c4bbf5d3c901d5db21261fa4b1137/airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py#L67-L69
So what do we do about it?
One of the approaches that I decided to take was to inspect the method `_convert_kube_model_object`. To my surprise, it doesn't use old class for anything but basically print of exception in "catch-all-other-scenarios" way.
https://github.com/apache/airflow/blob/1e570229533c4bbf5d3c901d5db21261fa4b1137/airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py#L28-L35
The minimal effort solution is to avoid import of legacy classes in backcompat altogether, while passing just the "string" name of those classes. This way the behavior of underline method is not changing, we still get the deprecation warnings when user is trying to import legacy classes separately in his code (while constructing the KPO), but there won't be warning shown during "processing" of KPO itself (on scheduler I assume?).
The proposed solution is potentially anti-pattern, as we pass literally the wrong type to defined function, but I'm ready to listen for constructive "better" solutions that will introduce minimal regressions to existing airflow codebase. That might come with extra overhead of validation of incoming type above the call to `_convert_kube_model_object` or such. In proposed solution should be no such extra overhead.
--
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 #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
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-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 #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimon222 commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r763440603
##########
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've applied alternative approach in separate PR - it will show warnings only if user imports the class in his code:
https://github.com/apache/airflow/pull/20031
This specific modification we do here in current PR is to actually stop printing deprecation when everything is GOOD (right now it will print it always, even if we don't use old classes - just because of how project was structured)
--
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] dimon222 commented on pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#issuecomment-995971824
Can someone retrigger CI for this PR? I think it didn't finish afterall.
--
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] jedcunningham commented on pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#issuecomment-995977304
Done. (Closing/reopening is an easy way to do it 👍)
--
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] dimon222 edited a comment on pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 edited a comment on pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#issuecomment-982085070
@jedcunningham this is where stuff might get sketchy.
I can apply those corrections, but because of inconsistency of current implementation, there will be.. corner case scenarios.
Specifically for Pod, Resources and PodRuntimeInfoEnv I don't see deprecation warnings in respective classes - I can add them, same as we do for others. However, the Resources logic even goes further by doing blind conversion without letting anyone know.
https://github.com/apache/airflow/blob/7b03678ec557a23017a6aa1fc225bbc84e5a92fb/airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py#L75-L76
--
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 #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
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-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 #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimon222 commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r763427426
##########
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:
We're converting it on the fly. So old class is not a blocker but rather a suggestion if user provided something completely unrelated.
--
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 #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r763495974
##########
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:
I don't think we need to show 2 deprecation warnings, and I agree with @dimon222 that it's implied the backcompat conversion will eventually 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-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 #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#issuecomment-982057375
I'm generally okay with this path, however, it should be done for everything, not just volumes and mounts. @dimon222, can you refactor the rest too? Probably worth renaming the kwarg in _convert_kube_model_object to be clear it is the name, not the actual 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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimon222 edited a comment on pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 edited a comment on pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#issuecomment-982085070
@jedcunningham this is where stuff might get sketchy.
I can apply those corrections, but because of inconsistency of current implementation, there will be.. corner case scenarios.
Specifically for Pod, Resources and PodRuntimeInfoEnv I don't see deprecation warnings for classes - I can add them, same as we do for others. However, the Resources logic even goes further by doing blind conversion without letting anyone know.
https://github.com/apache/airflow/blob/7b03678ec557a23017a6aa1fc225bbc84e5a92fb/airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py#L75-L76
--
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] dimon222 commented on pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#issuecomment-984733690
@jedcunningham if possible, some recommendation would be preferable. Seems like this kind of import that I added in last commit is not passing one of static checks.
--
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] jedcunningham merged pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
jedcunningham merged pull request #19726:
URL: https://github.com/apache/airflow/pull/19726
--
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] dimon222 commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r763440603
##########
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've applied alternative approach in separate PR - it will show warnings only if user imports the class in **his** code:
https://github.com/apache/airflow/pull/20031
This specific modification we do here in current PR is to actually stop printing deprecation when everything is GOOD (right now it will print it always, even if we don't use old classes - just because of how project was structured)
PS: however, I see no problem if we keep adding extra in this code, as entire compat will be gone at some point
--
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] dimon222 commented on pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#issuecomment-987332592
PS: don't merge until tests are done, there might be some broken after my last commit
--
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] dimon222 commented on pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#issuecomment-982076952
@jedcunningham sure, done
--
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] jedcunningham commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r762057039
##########
File path: airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py
##########
@@ -77,8 +71,10 @@ def convert_resources(resources) -> k8s.V1ResourceRequirements:
:return: k8s.V1ResourceRequirements
"""
if isinstance(resources, dict):
+ from airflow.providers.cncf.kubernetes.backcompat.pod import Resources
+
Review comment:
Ugh, and this removed the line completely... Can you just add an empty line after this import locally so we don't have to battle github suggestions?
--
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] dimon222 commented on pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#issuecomment-985786503
I think CI broke
--
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] dimon222 commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r763474581
##########
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"?
--
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 #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimon222 commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r763440603
##########
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've applied alternative approach in separate - it will show warnings only if user imports the class:
https://github.com/apache/airflow/pull/20031
This specific modification we do here is to actually stop printing deprecation when its not present (right now it will print it always, even if we don't use old classes - just because of how project was structured)
--
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] dimon222 commented on pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#issuecomment-987194017
@jedcunningham this is passing now, and I split the deprecation task for #20031 which might require same attention (i'm not sure how to solve the linter stuff without changing header)
--
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] jedcunningham commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r761352106
##########
File path: airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py
##########
@@ -77,8 +71,10 @@ def convert_resources(resources) -> k8s.V1ResourceRequirements:
:return: k8s.V1ResourceRequirements
"""
if isinstance(resources, dict):
+ from airflow.providers.cncf.kubernetes.backcompat.pod import Resources
+
Review comment:
```suggestion
```
Looks like github added spaces with the suggestion. My bad.
I'd also suggest that you [set up the pre-commit hooks](https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst), they help catch these issues early 👍.
--
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] dimon222 commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r763413828
##########
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:
Its to print exception below only
--
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] jedcunningham commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r768197737
##########
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:
At least in my eyes, "backcompat" and "convert" is the same thing 🤷♂️. I don't think we need to warn about both, but the nuance may be more important for end users.
In cases where there users aren't using the "old" classes directly, like your `resources` example (which will
actually get a deprecation warning in #20031), I think we should swallow the deprecation warning and emit a more helpful one for the user.
For example, for users using dict resources, instead of saying:
```
This module is deprecated. Please use `kubernetes.client.models.V1ResourceRequirements` and `kubernetes.client.models.V1ContainerPort`."
```
say (or something like it):
```
Setting resources as a dict is deprecated. Please use `kubernetes.client.models.V1ResourceRequirements`
```
`env_vars` also has this issue, not sure if there are others though.
Either way, I think the new deprecation warning stuff should be handled in #20031.
--
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] jedcunningham closed pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
jedcunningham closed pull request #19726:
URL: https://github.com/apache/airflow/pull/19726
--
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] jedcunningham commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r761216381
##########
File path: airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py
##########
@@ -77,8 +71,9 @@ def convert_resources(resources) -> k8s.V1ResourceRequirements:
:return: k8s.V1ResourceRequirements
"""
if isinstance(resources, dict):
+ from airflow.providers.cncf.kubernetes.backcompat.pod import Resources
Review comment:
```suggestion
from airflow.providers.cncf.kubernetes.backcompat.pod import Resources
```
This will fix the static check.
--
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] dimon222 commented on pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#issuecomment-982085070
@jedcunningham this is where stuff might get sketchy.
I can apply those corrections, but because of inconsistency of implementation, there will be.. corner case scenarios.
Specifically for Pod, Resources and PodRuntimeInfoEnv I don't see deprecation warnings for classes - I can add them, same as we do for others. However, the Resources logic even goes further by doing blind conversion without letting anyone know.
https://github.com/apache/airflow/blob/7b03678ec557a23017a6aa1fc225bbc84e5a92fb/airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py#L75-L76
--
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] dimon222 commented on pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#issuecomment-984873500
Still same static check test is crashing
--
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] dimon222 edited a comment on pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 edited a comment on pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#issuecomment-987194017
@jedcunningham this is passing now, and I split the deprecation task to #20031 which might require same attention (i'm not sure how to solve the linter stuff without changing header)
@potiuk FYI if u plan to include it in this RC or future ones.
--
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 #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimon222 commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r763427426
##########
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:
We're converting it on the fly (by checking if method for export as k8s class is present). So old class is not an "error" but rather an option if user provided something completely unrelated in there (at least based on what I understood)
--
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] dimon222 commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r763440603
##########
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've applied alternative approach in separate PR - it will show warnings only if user imports the class in **his** code:
https://github.com/apache/airflow/pull/20031
This specific modification we do here in current PR is to actually stop printing deprecation when everything is GOOD (right now it will print it always, even if we don't use old classes - just because of how project was structured)
--
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] dimon222 commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r763467499
##########
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:
Fully reasonable. Okay, I removed the argument altogether and instead changed recommendation to only use new class. Whoever is using the old class will get necessary deprecation warning from "imports" and the logic will blindly convert old classes. However, if user provides something unrelated, it will recommend only new class to use (which is okay anyway).
PS: Please don't merge until tests are verified. I'm not sure if those will require modification now because of different set of args.
--
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] jedcunningham commented on pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#issuecomment-982080561
Shouldn't we also remove the imports for the backcompat `Port`, `Resources` and `PodRuntimeInfoEnv`?
--
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] dimon222 commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r763427426
##########
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:
We're converting it on the fly (by checking if method for export as k8s class is present). So old class is not a blocker but rather a suggestion if user provided something completely unrelated in there (at least based on what I understood)
--
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] dimon222 commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r763467499
##########
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:
Fully reasonable. Okay, I removed the argument altogether and instead changed recommendation to only use new class. Whoever is using the old class will get necessary deprecation warning from "imports" and the logic will blindly convert old classes. However, if user provides something unrelated, it will recommend only new class to use (which is okay anyway).
--
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 #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimon222 commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r763440603
##########
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've applied alternative approach in separate PR - it will show warnings only if user imports the class in his code:
https://github.com/apache/airflow/pull/20031
This specific modification we do here is to actually stop printing deprecation when its not present (right now it will print it always, even if we don't use old classes - just because of how project was structured)
--
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] dimon222 commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r768182727
##########
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 got any better 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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimon222 commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r768204827
##########
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:
I guess thats a problem of the fact that Resources and Port are combined together under not straightforward named "pod.py", otherwise the filename itself should describe the exact concerning class (ala resources.py and port.py). But you're right, its better be handled separately in #20031. So, should we merge current PR then as is?
--
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] dimon222 edited a comment on pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 edited a comment on pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#issuecomment-995971824
Can someone retrigger CI for this PR? I think it didn't finish afterall. I believe aside of that this PR is ready for merge (unless any other concerns?)
--
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 #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#issuecomment-978376223
cc: @dimberman ?
--
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 #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#issuecomment-984746386
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] dimon222 edited a comment on pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 edited a comment on pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#issuecomment-987194017
@jedcunningham this is passing now, and I split the deprecation task to #20031 which might require same attention (i'm not sure how to solve the linter stuff without changing header)
--
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] dimon222 commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r763415460
##########
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.
--
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] dimon222 commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r763440603
##########
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've applied alternative approach in separate PR - it will show warnings only if user imports the class in his code:
https://github.com/apache/airflow/pull/20031
This specific modification we do here is to actually stop printing deprecation when everything is GOOD (right now it will print it always, even if we don't use old classes - just because of how project was structured)
--
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] dimon222 commented on a change in pull request #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19726:
URL: https://github.com/apache/airflow/pull/19726#discussion_r763440603
##########
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've applied alternative approach in separate PR - it will show warnings only if user imports the class:
https://github.com/apache/airflow/pull/20031
This specific modification we do here is to actually stop printing deprecation when its not present (right now it will print it always, even if we don't use old classes - just because of how project was structured)
--
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 #19726: Fix Volume/VolumeMount KPO DeprecationWarning
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org