-
Notifications
You must be signed in to change notification settings - Fork 14.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Volume/VolumeMount KPO DeprecationWarning #19726
Fix Volume/VolumeMount KPO DeprecationWarning #19726
Conversation
cc: @dimberman ? |
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? |
@jedcunningham sure, done |
Shouldn't we also remove the imports for the backcompat |
@jedcunningham this is where stuff might get sketchy. 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. |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Let's handle adding deprecation warnings in a separate PR.
airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py
Show resolved
Hide resolved
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. |
Still same static check test is crashing |
airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py
Outdated
Show resolved
Hide resolved
I think CI broke |
…converters.py Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
…converters.py Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
e42a05e
to
bce1a89
Compare
@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. |
airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
PS: don't merge until tests are done, there might be some broken after my last commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on second thought just want @jedcunningham's thoughts on whether we should also add deprecation here
|
||
|
||
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jedcunningham, do you think we should add deprecation warning here (i.e. before calling to_k8s_client_obj
) in addition to here?
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS: Isn't it implied by definition of "Deprecation"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jedcunningham got any better suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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?) |
Done. (Closing/reopening is an easy way to do it 👍) |
What is this?
This is to resolve incomplete solution of this commit 42e13e1 (#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:
airflow/airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py
Lines 246 to 247 in 1e57022
airflow/airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py
Lines 55 to 57 in 1e57022
airflow/airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py
Lines 67 to 69 in 1e57022
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.airflow/airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py
Lines 28 to 35 in 1e57022
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.