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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
