Skip to content

Enforce deprecation message format with EOL for google provider package - #41637

Merged
potiuk merged 2 commits into
apache:mainfrom
VladaZakharova:deprecation_eol_precommit
Sep 1, 2024
Merged

Enforce deprecation message format with EOL for google provider package#41637
potiuk merged 2 commits into
apache:mainfrom
VladaZakharova:deprecation_eol_precommit

Conversation

@moiseenkov

Copy link
Copy Markdown
Contributor

This PR introduces unification for deprecation messages formatting within Google's provider package by following:

  1. Updated the pre-commit hook check-code-deprecations so it enforces a specific message structure containing the end of life date for the deprecated entity. According to the deprecation policy discussed here, this deadline doesn't enforce removal of the deprecated entity from the source code, it only increases transparency and predictability for users, so they knew how much time do they have for the changes on their side.
  2. Another reason for this change is preparation for the cleaning-up process as Google's provider package has collected a lot of old legacy code and we'd be happy to finally clean it up.
  3. These changes work only within an airflow/providers/google scope.
  4. The default EOL is at least six months ahead.
  5. Some classes and methods have assigned earlier deadlines because they have been deprecated for a very long time already and can be removed earlier or their API is shut down and they are not working already.

@boring-cyborgboring-cyborgBot added area:dev-tools area:providers area:secrets provider:cncf-kubernetes Kubernetes (k8s) provider related issues provider:google Google (including GCP) related issues labels Aug 21, 2024
@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from 5e43f9e to bd97094CompareAugust 21, 2024 10:37

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NICE! I like it a lot.

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from bd97094 to 8f471c1CompareAugust 21, 2024 10:39
@potiuk

Copy link
Copy Markdown
Member

cc: @dstandish -> You wanted to do something like that - I think the approach proposed by the Google team (for now in google provider is really nice and we could apply it everywhere)

@ashb

ashb commented Aug 21, 2024

Copy link
Copy Markdown
Member

We should use yyyy-mm-dd (or yyyy.mm.dd, don't mind separator) so the date is unabigious

Comment threadairflow/providers/google/cloud/hooks/kubernetes_engine.py Outdated
@ashb

ashb commented Aug 21, 2024

Copy link
Copy Markdown
Member

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator:

@deprecated(reason="Please use `create_build_without_waiting_for_result`",removed_after="2025-03-01",category=AirflowProviderDeprecationWarning, )

That way the message can be generated programaticaly and be consistent that way -- no need for a precommit check then?

@kacpermuda

Copy link
Copy Markdown
Collaborator

Any standarization to deprecations is a good idea 🚀

did you consider adding extra (required) args to the deprecated decorator

Something related and still on my todo list (for whenever i have time) is #36952 where i wanted to add a custom decorators (or other construct) for different deprecations (function, arg, arg_value etc.), where we could separate these specific parts (what is deprecated, since when, removal date, what to use instead etc.) into separate arguments so it's easier to generate docs out of it. I think it could work even better now with common.compat provider, maybe we could implement something more global for the airflow codebase 😄

@potiuk

potiuk commented Aug 21, 2024

Copy link
Copy Markdown
Member

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator

Pre-commit check is still needed regardless - because we want to make sure that all deprecations have it. But yes - they could be better structured. one problem with adding new fields - is that since providers and airflow core are separated, anything "common" we come up with will have to go to "common.compat" - especially in the light that providers should be compatible with both Airflow 2 and Airflow 3.

So generally - yes better structure at the expense of more cross-provider dependencies.

@kacpermuda

Copy link
Copy Markdown
Collaborator

is that since providers and airflow core are separated, anything "common" we come up with will have to go to "common.compat" - especially in the light that providers should be compatible with both Airflow 2 and Airflow 3.

It's also explained in this discussion: #37075 (comment), maybe someone will find it useful, so just leaving it here.

@potiuk

Copy link
Copy Markdown
Member

It's also explained in this discussion: #37075 (comment), maybe someone will find it useful, so just leaving it here.

Yep. We've been discussing this :)

@moiseenkov

moiseenkov commented Aug 21, 2024

Copy link
Copy Markdown
ContributorAuthor

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator:

@deprecated(reason="Please use `create_build_without_waiting_for_result`",removed_after="2025-03-01",category=AirflowProviderDeprecationWarning, )

That way the message can be generated programaticaly and be consistent that way -- no need for a precommit check then?

This is the perfect approach, and I'd be happy to apply it, but as it was discussed in #37075 (comment), implementing this new decorator on the Airflow core side would make it very difficult to use in providers as they would have to bump Airflow version in their requirements.

The idea of releasing such a decorator in a separated provider package seems like a great solution! Is there any chance to implement it any time soon? I see that this work has already started #36952. @kacpermuda , @potiuk

Otherwise, our team would be happy to merge the current PR (with date format fixed) now in order to bring some order to the google provider package now. And once the new provider with the structured deprecation decorator is released, we could adopt the current changes accordingly.

@potiuk

Copy link
Copy Markdown
Member

The idea of releasing such a decorator in a separated provider package seems like a great solution! Is there any chance to implement it any time soon? I see that this work has already started #36952. @kacpermuda , @potiuk

The common.compat provider is already there. It's really a matter of:

  • adding the decorator there (with a note which minimum versio of compat provider has it).
  • bumping minor version of the compat provider
  • add a dependency in google provider to common.compat>=NEW_VERSION
  • any other provider that will want to use it, will have to do the same

We could also add this decorator to "airflow" utils with the note that it will be usable when min airflow in providers will be >= 3.0 (but then it should be added in airlfow-sdk (cc: @ashb) - so we should not do it now, but possibly leave a TODO to add it there.

@kacpermuda

Copy link
Copy Markdown
Collaborator

The common.compat provider is already there.

My main concern, though I’m not sure if it’s entirely valid, is that we previously discussed having a common.utils provider specifically designed to make it easier to add simple functionality, without external dependencies, to all providers, so that they do not have to rely on core Airflow version. The common.compat provider, as I understand, is meant to serve as a “proxy” that ensures code within providers functions without errors, with potentially fewer features, regardless of the Airflow version or other provider versions, but we should gradually remove things from there as we raise min. dependency versions f.e. for Airflow core. So it looks similar, but I'm not quite sure it's the same. Introducing this type of deprecation module into common.compat could make it more multi-functional, and it can be beneficial, I'm just not sure if i understand the background logic behind common.compat

@potiuk

potiuk commented Aug 21, 2024

Copy link
Copy Markdown
Member

So it looks similar, but I'm not quite sure it's the same. Introducing this type of deprecation module into common.compat could make it more multi-functional, and it can be beneficial, I'm just not sure if i understand the background logic behind common.compat

It is the same. I imagine (see above) that the decorator WILL become part of "airflow's sdk" eventually (whether it will be named as such or "task.sdk" - generally in Airflow 3 this SDK will become the "util" equivalent - and this SDK will contain everything that provider should be able to use (and in Airflow 3 providers will have no dependency on Airflow "core" - they will have dependency on the "sdk". So in this case "compat" is precisely compatibilty shim for the future "common" SDK that airlfow providers will be able to use.

The only problem is that we do not YET have the SDK - this is being worked on as part of AIP-72 and will likely come together with restructuring of whole Airlfow repo - including moving providers to separate sub-projects and so on, so it makes very little sense to implement it now in airflow 3, because it will anyhow change.

@ashb - do you agree with that assessment ?

And note for the future - since you proposed it here - we have to be aware that any kind of the common features that are supposed to be used in providers will have to eventually land in "airflow.sdk" that should be always future-compatible and for now they should land also in "common.compat" if we want to implement it in current providers, since we want the providers to work for both Airlfow 2/3.

Does it make sense what I described here? @ashb - is this your understanding as well?

@moiseenkov

Copy link
Copy Markdown
ContributorAuthor

@kacpermuda , hi,
Considering the conversation here it seems that the best way for us is promoting your implementation #36952 (as it is almost complete) and release it in the common.compat provider package.
Would you have time for re-opening your PR and adding support for sunset dates?

@shahar1shahar1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent idea!
I'm OK with merging it in its current form after resolving the dates formatting.
If you have time to adjust it to the common.compat format as suggested - you're welcome to tag me in the new PR and I'll be happy to review it.

@kacpermuda

Copy link
Copy Markdown
Collaborator

@moiseenkov, I’m happy to revisit #36952, but unfortunately, I can’t commit to a specific timeline at the moment. If you have any available resources, feel free to pick it up in the meantime. Regardless, great job with this PR and moving us closer to more standardized deprecations! :)

@potiuk

Copy link
Copy Markdown
Member

@moiseenkov, I’m happy to revisit #36952, but unfortunately, I can’t commit to a specific timeline at the moment. If you have any available resources, feel free to pick it up in the meantime. Regardless, great job with this PR and moving us closer to more standardized deprecations! :)

I think we can also make it in stages.

Stage 1. Implement it in google provider (with google-specific deprecated decorator - with time and pre-commit checks.
Stage 2. Learn from it and move it (possibly extending this to other cases like version mentioned by @dstandish ) -> either to common.compat or future airflow-sdk if it's ready and depended on by providers by that time.

Google provider is big enough to start "some" standardization efforts - many cases and likely > 50% of all operators and there is a history of things started there and adopted by others and made "common" (system tests for example)

I don't think we should aim to have a final solution implemented from day 0 that is applicable to all providers - having google as a good start and "experiment" is IMHO - totally acceptable.

WDYT others?

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch 2 times, most recently from 0794bde to fe269c0CompareAugust 26, 2024 10:19
@moiseenkov

moiseenkov commented Aug 26, 2024

Copy link
Copy Markdown
ContributorAuthor

Hi everyone,
As we agreed, I introduced a custom @deprecated decorator within the Google provider package with a detailed structure and pre-commit hook that enforces a sunset date for changes within the provider. The date format is also adjusted.

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from fe269c0 to c52f944CompareAugust 26, 2024 10:43
Comment threadairflow/providers/google/common/deprecated.py Outdated
@potiuk
potiukforce-pushed the deprecation_eol_precommit branch from c52f944 to f0418c4CompareAugust 29, 2024 15:03
@VladaZakharova

Copy link
Copy Markdown
Contributor

Hi @potiuk@eladkal@ashb !
I think we can try to merge this,m WDYT? The longer we wait, the more information about those deprecations become irrelevant haha

@potiuk

Copy link
Copy Markdown
Member

I am all for it @eladkal@dstandish - I think you had doubts - but I think we can do it as 'google provider specific' for now. I guess small, provider-specific solutions like that are better than inaction and waiting for responses. We can always change it later - no harm done to any other providers.

Will merge if I don't hear a complain

@shahar1shahar1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from f0418c4 to c941b57CompareAugust 30, 2024 14:23
@potiuk
potiuk merged commit 86e3d29 into apache:mainSep 1, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-toolsarea:providersarea:secretsprovider:cncf-kubernetesKubernetes (k8s) provider related issuesprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@moiseenkov@potiuk@ashb@kacpermuda@VladaZakharova@dstandish@eladkal@shahar1
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Enforce deprecation message format with EOL for google provider package by moiseenkov · Pull Request #41637 · apache/airflow · GitHub
Skip to content

Enforce deprecation message format with EOL for google provider package - #41637

Merged
potiuk merged 2 commits into
apache:mainfrom
VladaZakharova:deprecation_eol_precommit
Sep 1, 2024
Merged

Enforce deprecation message format with EOL for google provider package#41637
potiuk merged 2 commits into
apache:mainfrom
VladaZakharova:deprecation_eol_precommit

Conversation

@moiseenkov

Copy link
Copy Markdown
Contributor

This PR introduces unification for deprecation messages formatting within Google's provider package by following:

  1. Updated the pre-commit hook check-code-deprecations so it enforces a specific message structure containing the end of life date for the deprecated entity. According to the deprecation policy discussed here, this deadline doesn't enforce removal of the deprecated entity from the source code, it only increases transparency and predictability for users, so they knew how much time do they have for the changes on their side.
  2. Another reason for this change is preparation for the cleaning-up process as Google's provider package has collected a lot of old legacy code and we'd be happy to finally clean it up.
  3. These changes work only within an airflow/providers/google scope.
  4. The default EOL is at least six months ahead.
  5. Some classes and methods have assigned earlier deadlines because they have been deprecated for a very long time already and can be removed earlier or their API is shut down and they are not working already.

@boring-cyborgboring-cyborgBot added area:dev-tools area:providers area:secrets provider:cncf-kubernetes Kubernetes (k8s) provider related issues provider:google Google (including GCP) related issues labels Aug 21, 2024
@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from 5e43f9e to bd97094CompareAugust 21, 2024 10:37

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NICE! I like it a lot.

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from bd97094 to 8f471c1CompareAugust 21, 2024 10:39
@potiuk

Copy link
Copy Markdown
Member

cc: @dstandish -> You wanted to do something like that - I think the approach proposed by the Google team (for now in google provider is really nice and we could apply it everywhere)

@ashb

ashb commented Aug 21, 2024

Copy link
Copy Markdown
Member

We should use yyyy-mm-dd (or yyyy.mm.dd, don't mind separator) so the date is unabigious

Comment threadairflow/providers/google/cloud/hooks/kubernetes_engine.py Outdated
@ashb

ashb commented Aug 21, 2024

Copy link
Copy Markdown
Member

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator:

@deprecated(reason="Please use `create_build_without_waiting_for_result`",removed_after="2025-03-01",category=AirflowProviderDeprecationWarning, )

That way the message can be generated programaticaly and be consistent that way -- no need for a precommit check then?

@kacpermuda

Copy link
Copy Markdown
Collaborator

Any standarization to deprecations is a good idea 🚀

did you consider adding extra (required) args to the deprecated decorator

Something related and still on my todo list (for whenever i have time) is #36952 where i wanted to add a custom decorators (or other construct) for different deprecations (function, arg, arg_value etc.), where we could separate these specific parts (what is deprecated, since when, removal date, what to use instead etc.) into separate arguments so it's easier to generate docs out of it. I think it could work even better now with common.compat provider, maybe we could implement something more global for the airflow codebase 😄

@potiuk

potiuk commented Aug 21, 2024

Copy link
Copy Markdown
Member

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator

Pre-commit check is still needed regardless - because we want to make sure that all deprecations have it. But yes - they could be better structured. one problem with adding new fields - is that since providers and airflow core are separated, anything "common" we come up with will have to go to "common.compat" - especially in the light that providers should be compatible with both Airflow 2 and Airflow 3.

So generally - yes better structure at the expense of more cross-provider dependencies.

@kacpermuda

Copy link
Copy Markdown
Collaborator

is that since providers and airflow core are separated, anything "common" we come up with will have to go to "common.compat" - especially in the light that providers should be compatible with both Airflow 2 and Airflow 3.

It's also explained in this discussion: #37075 (comment), maybe someone will find it useful, so just leaving it here.

@potiuk

Copy link
Copy Markdown
Member

It's also explained in this discussion: #37075 (comment), maybe someone will find it useful, so just leaving it here.

Yep. We've been discussing this :)

@moiseenkov

moiseenkov commented Aug 21, 2024

Copy link
Copy Markdown
ContributorAuthor

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator:

@deprecated(reason="Please use `create_build_without_waiting_for_result`",removed_after="2025-03-01",category=AirflowProviderDeprecationWarning, )

That way the message can be generated programaticaly and be consistent that way -- no need for a precommit check then?

This is the perfect approach, and I'd be happy to apply it, but as it was discussed in #37075 (comment), implementing this new decorator on the Airflow core side would make it very difficult to use in providers as they would have to bump Airflow version in their requirements.

The idea of releasing such a decorator in a separated provider package seems like a great solution! Is there any chance to implement it any time soon? I see that this work has already started #36952. @kacpermuda , @potiuk

Otherwise, our team would be happy to merge the current PR (with date format fixed) now in order to bring some order to the google provider package now. And once the new provider with the structured deprecation decorator is released, we could adopt the current changes accordingly.

@potiuk

Copy link
Copy Markdown
Member

The idea of releasing such a decorator in a separated provider package seems like a great solution! Is there any chance to implement it any time soon? I see that this work has already started #36952. @kacpermuda , @potiuk

The common.compat provider is already there. It's really a matter of:

  • adding the decorator there (with a note which minimum versio of compat provider has it).
  • bumping minor version of the compat provider
  • add a dependency in google provider to common.compat>=NEW_VERSION
  • any other provider that will want to use it, will have to do the same

We could also add this decorator to "airflow" utils with the note that it will be usable when min airflow in providers will be >= 3.0 (but then it should be added in airlfow-sdk (cc: @ashb) - so we should not do it now, but possibly leave a TODO to add it there.

@kacpermuda

Copy link
Copy Markdown
Collaborator

The common.compat provider is already there.

My main concern, though I’m not sure if it’s entirely valid, is that we previously discussed having a common.utils provider specifically designed to make it easier to add simple functionality, without external dependencies, to all providers, so that they do not have to rely on core Airflow version. The common.compat provider, as I understand, is meant to serve as a “proxy” that ensures code within providers functions without errors, with potentially fewer features, regardless of the Airflow version or other provider versions, but we should gradually remove things from there as we raise min. dependency versions f.e. for Airflow core. So it looks similar, but I'm not quite sure it's the same. Introducing this type of deprecation module into common.compat could make it more multi-functional, and it can be beneficial, I'm just not sure if i understand the background logic behind common.compat

@potiuk

potiuk commented Aug 21, 2024

Copy link
Copy Markdown
Member

So it looks similar, but I'm not quite sure it's the same. Introducing this type of deprecation module into common.compat could make it more multi-functional, and it can be beneficial, I'm just not sure if i understand the background logic behind common.compat

It is the same. I imagine (see above) that the decorator WILL become part of "airflow's sdk" eventually (whether it will be named as such or "task.sdk" - generally in Airflow 3 this SDK will become the "util" equivalent - and this SDK will contain everything that provider should be able to use (and in Airflow 3 providers will have no dependency on Airflow "core" - they will have dependency on the "sdk". So in this case "compat" is precisely compatibilty shim for the future "common" SDK that airlfow providers will be able to use.

The only problem is that we do not YET have the SDK - this is being worked on as part of AIP-72 and will likely come together with restructuring of whole Airlfow repo - including moving providers to separate sub-projects and so on, so it makes very little sense to implement it now in airflow 3, because it will anyhow change.

@ashb - do you agree with that assessment ?

And note for the future - since you proposed it here - we have to be aware that any kind of the common features that are supposed to be used in providers will have to eventually land in "airflow.sdk" that should be always future-compatible and for now they should land also in "common.compat" if we want to implement it in current providers, since we want the providers to work for both Airlfow 2/3.

Does it make sense what I described here? @ashb - is this your understanding as well?

@moiseenkov

Copy link
Copy Markdown
ContributorAuthor

@kacpermuda , hi,
Considering the conversation here it seems that the best way for us is promoting your implementation #36952 (as it is almost complete) and release it in the common.compat provider package.
Would you have time for re-opening your PR and adding support for sunset dates?

@shahar1shahar1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent idea!
I'm OK with merging it in its current form after resolving the dates formatting.
If you have time to adjust it to the common.compat format as suggested - you're welcome to tag me in the new PR and I'll be happy to review it.

@kacpermuda

Copy link
Copy Markdown
Collaborator

@moiseenkov, I’m happy to revisit #36952, but unfortunately, I can’t commit to a specific timeline at the moment. If you have any available resources, feel free to pick it up in the meantime. Regardless, great job with this PR and moving us closer to more standardized deprecations! :)

@potiuk

Copy link
Copy Markdown
Member

@moiseenkov, I’m happy to revisit #36952, but unfortunately, I can’t commit to a specific timeline at the moment. If you have any available resources, feel free to pick it up in the meantime. Regardless, great job with this PR and moving us closer to more standardized deprecations! :)

I think we can also make it in stages.

Stage 1. Implement it in google provider (with google-specific deprecated decorator - with time and pre-commit checks.
Stage 2. Learn from it and move it (possibly extending this to other cases like version mentioned by @dstandish ) -> either to common.compat or future airflow-sdk if it's ready and depended on by providers by that time.

Google provider is big enough to start "some" standardization efforts - many cases and likely > 50% of all operators and there is a history of things started there and adopted by others and made "common" (system tests for example)

I don't think we should aim to have a final solution implemented from day 0 that is applicable to all providers - having google as a good start and "experiment" is IMHO - totally acceptable.

WDYT others?

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch 2 times, most recently from 0794bde to fe269c0CompareAugust 26, 2024 10:19
@moiseenkov

moiseenkov commented Aug 26, 2024

Copy link
Copy Markdown
ContributorAuthor

Hi everyone,
As we agreed, I introduced a custom @deprecated decorator within the Google provider package with a detailed structure and pre-commit hook that enforces a sunset date for changes within the provider. The date format is also adjusted.

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from fe269c0 to c52f944CompareAugust 26, 2024 10:43
Comment threadairflow/providers/google/common/deprecated.py Outdated
@potiuk
potiukforce-pushed the deprecation_eol_precommit branch from c52f944 to f0418c4CompareAugust 29, 2024 15:03
@VladaZakharova

Copy link
Copy Markdown
Contributor

Hi @potiuk@eladkal@ashb !
I think we can try to merge this,m WDYT? The longer we wait, the more information about those deprecations become irrelevant haha

@potiuk

Copy link
Copy Markdown
Member

I am all for it @eladkal@dstandish - I think you had doubts - but I think we can do it as 'google provider specific' for now. I guess small, provider-specific solutions like that are better than inaction and waiting for responses. We can always change it later - no harm done to any other providers.

Will merge if I don't hear a complain

@shahar1shahar1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from f0418c4 to c941b57CompareAugust 30, 2024 14:23
@potiuk
potiuk merged commit 86e3d29 into apache:mainSep 1, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-toolsarea:providersarea:secretsprovider:cncf-kubernetesKubernetes (k8s) provider related issuesprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@moiseenkov@potiuk@ashb@kacpermuda@VladaZakharova@dstandish@eladkal@shahar1
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Enforce deprecation message format with EOL for google provider package by moiseenkov · Pull Request #41637 · apache/airflow · GitHub
Skip to content

Enforce deprecation message format with EOL for google provider package - #41637

Merged
potiuk merged 2 commits into
apache:mainfrom
VladaZakharova:deprecation_eol_precommit
Sep 1, 2024
Merged

Enforce deprecation message format with EOL for google provider package#41637
potiuk merged 2 commits into
apache:mainfrom
VladaZakharova:deprecation_eol_precommit

Conversation

@moiseenkov

Copy link
Copy Markdown
Contributor

This PR introduces unification for deprecation messages formatting within Google's provider package by following:

  1. Updated the pre-commit hook check-code-deprecations so it enforces a specific message structure containing the end of life date for the deprecated entity. According to the deprecation policy discussed here, this deadline doesn't enforce removal of the deprecated entity from the source code, it only increases transparency and predictability for users, so they knew how much time do they have for the changes on their side.
  2. Another reason for this change is preparation for the cleaning-up process as Google's provider package has collected a lot of old legacy code and we'd be happy to finally clean it up.
  3. These changes work only within an airflow/providers/google scope.
  4. The default EOL is at least six months ahead.
  5. Some classes and methods have assigned earlier deadlines because they have been deprecated for a very long time already and can be removed earlier or their API is shut down and they are not working already.

@boring-cyborgboring-cyborgBot added area:dev-tools area:providers area:secrets provider:cncf-kubernetes Kubernetes (k8s) provider related issues provider:google Google (including GCP) related issues labels Aug 21, 2024
@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from 5e43f9e to bd97094CompareAugust 21, 2024 10:37

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NICE! I like it a lot.

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from bd97094 to 8f471c1CompareAugust 21, 2024 10:39
@potiuk

Copy link
Copy Markdown
Member

cc: @dstandish -> You wanted to do something like that - I think the approach proposed by the Google team (for now in google provider is really nice and we could apply it everywhere)

@ashb

ashb commented Aug 21, 2024

Copy link
Copy Markdown
Member

We should use yyyy-mm-dd (or yyyy.mm.dd, don't mind separator) so the date is unabigious

Comment threadairflow/providers/google/cloud/hooks/kubernetes_engine.py Outdated
@ashb

ashb commented Aug 21, 2024

Copy link
Copy Markdown
Member

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator:

@deprecated(reason="Please use `create_build_without_waiting_for_result`",removed_after="2025-03-01",category=AirflowProviderDeprecationWarning, )

That way the message can be generated programaticaly and be consistent that way -- no need for a precommit check then?

@kacpermuda

Copy link
Copy Markdown
Collaborator

Any standarization to deprecations is a good idea 🚀

did you consider adding extra (required) args to the deprecated decorator

Something related and still on my todo list (for whenever i have time) is #36952 where i wanted to add a custom decorators (or other construct) for different deprecations (function, arg, arg_value etc.), where we could separate these specific parts (what is deprecated, since when, removal date, what to use instead etc.) into separate arguments so it's easier to generate docs out of it. I think it could work even better now with common.compat provider, maybe we could implement something more global for the airflow codebase 😄

@potiuk

potiuk commented Aug 21, 2024

Copy link
Copy Markdown
Member

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator

Pre-commit check is still needed regardless - because we want to make sure that all deprecations have it. But yes - they could be better structured. one problem with adding new fields - is that since providers and airflow core are separated, anything "common" we come up with will have to go to "common.compat" - especially in the light that providers should be compatible with both Airflow 2 and Airflow 3.

So generally - yes better structure at the expense of more cross-provider dependencies.

@kacpermuda

Copy link
Copy Markdown
Collaborator

is that since providers and airflow core are separated, anything "common" we come up with will have to go to "common.compat" - especially in the light that providers should be compatible with both Airflow 2 and Airflow 3.

It's also explained in this discussion: #37075 (comment), maybe someone will find it useful, so just leaving it here.

@potiuk

Copy link
Copy Markdown
Member

It's also explained in this discussion: #37075 (comment), maybe someone will find it useful, so just leaving it here.

Yep. We've been discussing this :)

@moiseenkov

moiseenkov commented Aug 21, 2024

Copy link
Copy Markdown
ContributorAuthor

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator:

@deprecated(reason="Please use `create_build_without_waiting_for_result`",removed_after="2025-03-01",category=AirflowProviderDeprecationWarning, )

That way the message can be generated programaticaly and be consistent that way -- no need for a precommit check then?

This is the perfect approach, and I'd be happy to apply it, but as it was discussed in #37075 (comment), implementing this new decorator on the Airflow core side would make it very difficult to use in providers as they would have to bump Airflow version in their requirements.

The idea of releasing such a decorator in a separated provider package seems like a great solution! Is there any chance to implement it any time soon? I see that this work has already started #36952. @kacpermuda , @potiuk

Otherwise, our team would be happy to merge the current PR (with date format fixed) now in order to bring some order to the google provider package now. And once the new provider with the structured deprecation decorator is released, we could adopt the current changes accordingly.

@potiuk

Copy link
Copy Markdown
Member

The idea of releasing such a decorator in a separated provider package seems like a great solution! Is there any chance to implement it any time soon? I see that this work has already started #36952. @kacpermuda , @potiuk

The common.compat provider is already there. It's really a matter of:

  • adding the decorator there (with a note which minimum versio of compat provider has it).
  • bumping minor version of the compat provider
  • add a dependency in google provider to common.compat>=NEW_VERSION
  • any other provider that will want to use it, will have to do the same

We could also add this decorator to "airflow" utils with the note that it will be usable when min airflow in providers will be >= 3.0 (but then it should be added in airlfow-sdk (cc: @ashb) - so we should not do it now, but possibly leave a TODO to add it there.

@kacpermuda

Copy link
Copy Markdown
Collaborator

The common.compat provider is already there.

My main concern, though I’m not sure if it’s entirely valid, is that we previously discussed having a common.utils provider specifically designed to make it easier to add simple functionality, without external dependencies, to all providers, so that they do not have to rely on core Airflow version. The common.compat provider, as I understand, is meant to serve as a “proxy” that ensures code within providers functions without errors, with potentially fewer features, regardless of the Airflow version or other provider versions, but we should gradually remove things from there as we raise min. dependency versions f.e. for Airflow core. So it looks similar, but I'm not quite sure it's the same. Introducing this type of deprecation module into common.compat could make it more multi-functional, and it can be beneficial, I'm just not sure if i understand the background logic behind common.compat

@potiuk

potiuk commented Aug 21, 2024

Copy link
Copy Markdown
Member

So it looks similar, but I'm not quite sure it's the same. Introducing this type of deprecation module into common.compat could make it more multi-functional, and it can be beneficial, I'm just not sure if i understand the background logic behind common.compat

It is the same. I imagine (see above) that the decorator WILL become part of "airflow's sdk" eventually (whether it will be named as such or "task.sdk" - generally in Airflow 3 this SDK will become the "util" equivalent - and this SDK will contain everything that provider should be able to use (and in Airflow 3 providers will have no dependency on Airflow "core" - they will have dependency on the "sdk". So in this case "compat" is precisely compatibilty shim for the future "common" SDK that airlfow providers will be able to use.

The only problem is that we do not YET have the SDK - this is being worked on as part of AIP-72 and will likely come together with restructuring of whole Airlfow repo - including moving providers to separate sub-projects and so on, so it makes very little sense to implement it now in airflow 3, because it will anyhow change.

@ashb - do you agree with that assessment ?

And note for the future - since you proposed it here - we have to be aware that any kind of the common features that are supposed to be used in providers will have to eventually land in "airflow.sdk" that should be always future-compatible and for now they should land also in "common.compat" if we want to implement it in current providers, since we want the providers to work for both Airlfow 2/3.

Does it make sense what I described here? @ashb - is this your understanding as well?

@moiseenkov

Copy link
Copy Markdown
ContributorAuthor

@kacpermuda , hi,
Considering the conversation here it seems that the best way for us is promoting your implementation #36952 (as it is almost complete) and release it in the common.compat provider package.
Would you have time for re-opening your PR and adding support for sunset dates?

@shahar1shahar1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent idea!
I'm OK with merging it in its current form after resolving the dates formatting.
If you have time to adjust it to the common.compat format as suggested - you're welcome to tag me in the new PR and I'll be happy to review it.

@kacpermuda

Copy link
Copy Markdown
Collaborator

@moiseenkov, I’m happy to revisit #36952, but unfortunately, I can’t commit to a specific timeline at the moment. If you have any available resources, feel free to pick it up in the meantime. Regardless, great job with this PR and moving us closer to more standardized deprecations! :)

@potiuk

Copy link
Copy Markdown
Member

@moiseenkov, I’m happy to revisit #36952, but unfortunately, I can’t commit to a specific timeline at the moment. If you have any available resources, feel free to pick it up in the meantime. Regardless, great job with this PR and moving us closer to more standardized deprecations! :)

I think we can also make it in stages.

Stage 1. Implement it in google provider (with google-specific deprecated decorator - with time and pre-commit checks.
Stage 2. Learn from it and move it (possibly extending this to other cases like version mentioned by @dstandish ) -> either to common.compat or future airflow-sdk if it's ready and depended on by providers by that time.

Google provider is big enough to start "some" standardization efforts - many cases and likely > 50% of all operators and there is a history of things started there and adopted by others and made "common" (system tests for example)

I don't think we should aim to have a final solution implemented from day 0 that is applicable to all providers - having google as a good start and "experiment" is IMHO - totally acceptable.

WDYT others?

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch 2 times, most recently from 0794bde to fe269c0CompareAugust 26, 2024 10:19
@moiseenkov

moiseenkov commented Aug 26, 2024

Copy link
Copy Markdown
ContributorAuthor

Hi everyone,
As we agreed, I introduced a custom @deprecated decorator within the Google provider package with a detailed structure and pre-commit hook that enforces a sunset date for changes within the provider. The date format is also adjusted.

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from fe269c0 to c52f944CompareAugust 26, 2024 10:43
Comment threadairflow/providers/google/common/deprecated.py Outdated
@potiuk
potiukforce-pushed the deprecation_eol_precommit branch from c52f944 to f0418c4CompareAugust 29, 2024 15:03
@VladaZakharova

Copy link
Copy Markdown
Contributor

Hi @potiuk@eladkal@ashb !
I think we can try to merge this,m WDYT? The longer we wait, the more information about those deprecations become irrelevant haha

@potiuk

Copy link
Copy Markdown
Member

I am all for it @eladkal@dstandish - I think you had doubts - but I think we can do it as 'google provider specific' for now. I guess small, provider-specific solutions like that are better than inaction and waiting for responses. We can always change it later - no harm done to any other providers.

Will merge if I don't hear a complain

@shahar1shahar1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from f0418c4 to c941b57CompareAugust 30, 2024 14:23
@potiuk
potiuk merged commit 86e3d29 into apache:mainSep 1, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-toolsarea:providersarea:secretsprovider:cncf-kubernetesKubernetes (k8s) provider related issuesprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@moiseenkov@potiuk@ashb@kacpermuda@VladaZakharova@dstandish@eladkal@shahar1
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Enforce deprecation message format with EOL for google provider package by moiseenkov · Pull Request #41637 · apache/airflow · GitHub
Skip to content

Enforce deprecation message format with EOL for google provider package - #41637

Merged
potiuk merged 2 commits into
apache:mainfrom
VladaZakharova:deprecation_eol_precommit
Sep 1, 2024
Merged

Enforce deprecation message format with EOL for google provider package#41637
potiuk merged 2 commits into
apache:mainfrom
VladaZakharova:deprecation_eol_precommit

Conversation

@moiseenkov

Copy link
Copy Markdown
Contributor

This PR introduces unification for deprecation messages formatting within Google's provider package by following:

  1. Updated the pre-commit hook check-code-deprecations so it enforces a specific message structure containing the end of life date for the deprecated entity. According to the deprecation policy discussed here, this deadline doesn't enforce removal of the deprecated entity from the source code, it only increases transparency and predictability for users, so they knew how much time do they have for the changes on their side.
  2. Another reason for this change is preparation for the cleaning-up process as Google's provider package has collected a lot of old legacy code and we'd be happy to finally clean it up.
  3. These changes work only within an airflow/providers/google scope.
  4. The default EOL is at least six months ahead.
  5. Some classes and methods have assigned earlier deadlines because they have been deprecated for a very long time already and can be removed earlier or their API is shut down and they are not working already.

@boring-cyborgboring-cyborgBot added area:dev-tools area:providers area:secrets provider:cncf-kubernetes Kubernetes (k8s) provider related issues provider:google Google (including GCP) related issues labels Aug 21, 2024
@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from 5e43f9e to bd97094CompareAugust 21, 2024 10:37

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NICE! I like it a lot.

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from bd97094 to 8f471c1CompareAugust 21, 2024 10:39
@potiuk

Copy link
Copy Markdown
Member

cc: @dstandish -> You wanted to do something like that - I think the approach proposed by the Google team (for now in google provider is really nice and we could apply it everywhere)

@ashb

ashb commented Aug 21, 2024

Copy link
Copy Markdown
Member

We should use yyyy-mm-dd (or yyyy.mm.dd, don't mind separator) so the date is unabigious

Comment threadairflow/providers/google/cloud/hooks/kubernetes_engine.py Outdated
@ashb

ashb commented Aug 21, 2024

Copy link
Copy Markdown
Member

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator:

@deprecated(reason="Please use `create_build_without_waiting_for_result`",removed_after="2025-03-01",category=AirflowProviderDeprecationWarning, )

That way the message can be generated programaticaly and be consistent that way -- no need for a precommit check then?

@kacpermuda

Copy link
Copy Markdown
Collaborator

Any standarization to deprecations is a good idea 🚀

did you consider adding extra (required) args to the deprecated decorator

Something related and still on my todo list (for whenever i have time) is #36952 where i wanted to add a custom decorators (or other construct) for different deprecations (function, arg, arg_value etc.), where we could separate these specific parts (what is deprecated, since when, removal date, what to use instead etc.) into separate arguments so it's easier to generate docs out of it. I think it could work even better now with common.compat provider, maybe we could implement something more global for the airflow codebase 😄

@potiuk

potiuk commented Aug 21, 2024

Copy link
Copy Markdown
Member

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator

Pre-commit check is still needed regardless - because we want to make sure that all deprecations have it. But yes - they could be better structured. one problem with adding new fields - is that since providers and airflow core are separated, anything "common" we come up with will have to go to "common.compat" - especially in the light that providers should be compatible with both Airflow 2 and Airflow 3.

So generally - yes better structure at the expense of more cross-provider dependencies.

@kacpermuda

Copy link
Copy Markdown
Collaborator

is that since providers and airflow core are separated, anything "common" we come up with will have to go to "common.compat" - especially in the light that providers should be compatible with both Airflow 2 and Airflow 3.

It's also explained in this discussion: #37075 (comment), maybe someone will find it useful, so just leaving it here.

@potiuk

Copy link
Copy Markdown
Member

It's also explained in this discussion: #37075 (comment), maybe someone will find it useful, so just leaving it here.

Yep. We've been discussing this :)

@moiseenkov

moiseenkov commented Aug 21, 2024

Copy link
Copy Markdown
ContributorAuthor

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator:

@deprecated(reason="Please use `create_build_without_waiting_for_result`",removed_after="2025-03-01",category=AirflowProviderDeprecationWarning, )

That way the message can be generated programaticaly and be consistent that way -- no need for a precommit check then?

This is the perfect approach, and I'd be happy to apply it, but as it was discussed in #37075 (comment), implementing this new decorator on the Airflow core side would make it very difficult to use in providers as they would have to bump Airflow version in their requirements.

The idea of releasing such a decorator in a separated provider package seems like a great solution! Is there any chance to implement it any time soon? I see that this work has already started #36952. @kacpermuda , @potiuk

Otherwise, our team would be happy to merge the current PR (with date format fixed) now in order to bring some order to the google provider package now. And once the new provider with the structured deprecation decorator is released, we could adopt the current changes accordingly.

@potiuk

Copy link
Copy Markdown
Member

The idea of releasing such a decorator in a separated provider package seems like a great solution! Is there any chance to implement it any time soon? I see that this work has already started #36952. @kacpermuda , @potiuk

The common.compat provider is already there. It's really a matter of:

  • adding the decorator there (with a note which minimum versio of compat provider has it).
  • bumping minor version of the compat provider
  • add a dependency in google provider to common.compat>=NEW_VERSION
  • any other provider that will want to use it, will have to do the same

We could also add this decorator to "airflow" utils with the note that it will be usable when min airflow in providers will be >= 3.0 (but then it should be added in airlfow-sdk (cc: @ashb) - so we should not do it now, but possibly leave a TODO to add it there.

@kacpermuda

Copy link
Copy Markdown
Collaborator

The common.compat provider is already there.

My main concern, though I’m not sure if it’s entirely valid, is that we previously discussed having a common.utils provider specifically designed to make it easier to add simple functionality, without external dependencies, to all providers, so that they do not have to rely on core Airflow version. The common.compat provider, as I understand, is meant to serve as a “proxy” that ensures code within providers functions without errors, with potentially fewer features, regardless of the Airflow version or other provider versions, but we should gradually remove things from there as we raise min. dependency versions f.e. for Airflow core. So it looks similar, but I'm not quite sure it's the same. Introducing this type of deprecation module into common.compat could make it more multi-functional, and it can be beneficial, I'm just not sure if i understand the background logic behind common.compat

@potiuk

potiuk commented Aug 21, 2024

Copy link
Copy Markdown
Member

So it looks similar, but I'm not quite sure it's the same. Introducing this type of deprecation module into common.compat could make it more multi-functional, and it can be beneficial, I'm just not sure if i understand the background logic behind common.compat

It is the same. I imagine (see above) that the decorator WILL become part of "airflow's sdk" eventually (whether it will be named as such or "task.sdk" - generally in Airflow 3 this SDK will become the "util" equivalent - and this SDK will contain everything that provider should be able to use (and in Airflow 3 providers will have no dependency on Airflow "core" - they will have dependency on the "sdk". So in this case "compat" is precisely compatibilty shim for the future "common" SDK that airlfow providers will be able to use.

The only problem is that we do not YET have the SDK - this is being worked on as part of AIP-72 and will likely come together with restructuring of whole Airlfow repo - including moving providers to separate sub-projects and so on, so it makes very little sense to implement it now in airflow 3, because it will anyhow change.

@ashb - do you agree with that assessment ?

And note for the future - since you proposed it here - we have to be aware that any kind of the common features that are supposed to be used in providers will have to eventually land in "airflow.sdk" that should be always future-compatible and for now they should land also in "common.compat" if we want to implement it in current providers, since we want the providers to work for both Airlfow 2/3.

Does it make sense what I described here? @ashb - is this your understanding as well?

@moiseenkov

Copy link
Copy Markdown
ContributorAuthor

@kacpermuda , hi,
Considering the conversation here it seems that the best way for us is promoting your implementation #36952 (as it is almost complete) and release it in the common.compat provider package.
Would you have time for re-opening your PR and adding support for sunset dates?

@shahar1shahar1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent idea!
I'm OK with merging it in its current form after resolving the dates formatting.
If you have time to adjust it to the common.compat format as suggested - you're welcome to tag me in the new PR and I'll be happy to review it.

@kacpermuda

Copy link
Copy Markdown
Collaborator

@moiseenkov, I’m happy to revisit #36952, but unfortunately, I can’t commit to a specific timeline at the moment. If you have any available resources, feel free to pick it up in the meantime. Regardless, great job with this PR and moving us closer to more standardized deprecations! :)

@potiuk

Copy link
Copy Markdown
Member

@moiseenkov, I’m happy to revisit #36952, but unfortunately, I can’t commit to a specific timeline at the moment. If you have any available resources, feel free to pick it up in the meantime. Regardless, great job with this PR and moving us closer to more standardized deprecations! :)

I think we can also make it in stages.

Stage 1. Implement it in google provider (with google-specific deprecated decorator - with time and pre-commit checks.
Stage 2. Learn from it and move it (possibly extending this to other cases like version mentioned by @dstandish ) -> either to common.compat or future airflow-sdk if it's ready and depended on by providers by that time.

Google provider is big enough to start "some" standardization efforts - many cases and likely > 50% of all operators and there is a history of things started there and adopted by others and made "common" (system tests for example)

I don't think we should aim to have a final solution implemented from day 0 that is applicable to all providers - having google as a good start and "experiment" is IMHO - totally acceptable.

WDYT others?

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch 2 times, most recently from 0794bde to fe269c0CompareAugust 26, 2024 10:19
@moiseenkov

moiseenkov commented Aug 26, 2024

Copy link
Copy Markdown
ContributorAuthor

Hi everyone,
As we agreed, I introduced a custom @deprecated decorator within the Google provider package with a detailed structure and pre-commit hook that enforces a sunset date for changes within the provider. The date format is also adjusted.

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from fe269c0 to c52f944CompareAugust 26, 2024 10:43
Comment threadairflow/providers/google/common/deprecated.py Outdated
@potiuk
potiukforce-pushed the deprecation_eol_precommit branch from c52f944 to f0418c4CompareAugust 29, 2024 15:03
@VladaZakharova

Copy link
Copy Markdown
Contributor

Hi @potiuk@eladkal@ashb !
I think we can try to merge this,m WDYT? The longer we wait, the more information about those deprecations become irrelevant haha

@potiuk

Copy link
Copy Markdown
Member

I am all for it @eladkal@dstandish - I think you had doubts - but I think we can do it as 'google provider specific' for now. I guess small, provider-specific solutions like that are better than inaction and waiting for responses. We can always change it later - no harm done to any other providers.

Will merge if I don't hear a complain

@shahar1shahar1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from f0418c4 to c941b57CompareAugust 30, 2024 14:23
@potiuk
potiuk merged commit 86e3d29 into apache:mainSep 1, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-toolsarea:providersarea:secretsprovider:cncf-kubernetesKubernetes (k8s) provider related issuesprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@moiseenkov@potiuk@ashb@kacpermuda@VladaZakharova@dstandish@eladkal@shahar1
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Enforce deprecation message format with EOL for google provider package by moiseenkov · Pull Request #41637 · apache/airflow · GitHub
Skip to content

Enforce deprecation message format with EOL for google provider package - #41637

Merged
potiuk merged 2 commits into
apache:mainfrom
VladaZakharova:deprecation_eol_precommit
Sep 1, 2024
Merged

Enforce deprecation message format with EOL for google provider package#41637
potiuk merged 2 commits into
apache:mainfrom
VladaZakharova:deprecation_eol_precommit

Conversation

@moiseenkov

Copy link
Copy Markdown
Contributor

This PR introduces unification for deprecation messages formatting within Google's provider package by following:

  1. Updated the pre-commit hook check-code-deprecations so it enforces a specific message structure containing the end of life date for the deprecated entity. According to the deprecation policy discussed here, this deadline doesn't enforce removal of the deprecated entity from the source code, it only increases transparency and predictability for users, so they knew how much time do they have for the changes on their side.
  2. Another reason for this change is preparation for the cleaning-up process as Google's provider package has collected a lot of old legacy code and we'd be happy to finally clean it up.
  3. These changes work only within an airflow/providers/google scope.
  4. The default EOL is at least six months ahead.
  5. Some classes and methods have assigned earlier deadlines because they have been deprecated for a very long time already and can be removed earlier or their API is shut down and they are not working already.

@boring-cyborgboring-cyborgBot added area:dev-tools area:providers area:secrets provider:cncf-kubernetes Kubernetes (k8s) provider related issues provider:google Google (including GCP) related issues labels Aug 21, 2024
@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from 5e43f9e to bd97094CompareAugust 21, 2024 10:37

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NICE! I like it a lot.

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from bd97094 to 8f471c1CompareAugust 21, 2024 10:39
@potiuk

Copy link
Copy Markdown
Member

cc: @dstandish -> You wanted to do something like that - I think the approach proposed by the Google team (for now in google provider is really nice and we could apply it everywhere)

@ashb

ashb commented Aug 21, 2024

Copy link
Copy Markdown
Member

We should use yyyy-mm-dd (or yyyy.mm.dd, don't mind separator) so the date is unabigious

Comment threadairflow/providers/google/cloud/hooks/kubernetes_engine.py Outdated
@ashb

ashb commented Aug 21, 2024

Copy link
Copy Markdown
Member

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator:

@deprecated(reason="Please use `create_build_without_waiting_for_result`",removed_after="2025-03-01",category=AirflowProviderDeprecationWarning, )

That way the message can be generated programaticaly and be consistent that way -- no need for a precommit check then?

@kacpermuda

Copy link
Copy Markdown
Collaborator

Any standarization to deprecations is a good idea 🚀

did you consider adding extra (required) args to the deprecated decorator

Something related and still on my todo list (for whenever i have time) is #36952 where i wanted to add a custom decorators (or other construct) for different deprecations (function, arg, arg_value etc.), where we could separate these specific parts (what is deprecated, since when, removal date, what to use instead etc.) into separate arguments so it's easier to generate docs out of it. I think it could work even better now with common.compat provider, maybe we could implement something more global for the airflow codebase 😄

@potiuk

potiuk commented Aug 21, 2024

Copy link
Copy Markdown
Member

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator

Pre-commit check is still needed regardless - because we want to make sure that all deprecations have it. But yes - they could be better structured. one problem with adding new fields - is that since providers and airflow core are separated, anything "common" we come up with will have to go to "common.compat" - especially in the light that providers should be compatible with both Airflow 2 and Airflow 3.

So generally - yes better structure at the expense of more cross-provider dependencies.

@kacpermuda

Copy link
Copy Markdown
Collaborator

is that since providers and airflow core are separated, anything "common" we come up with will have to go to "common.compat" - especially in the light that providers should be compatible with both Airflow 2 and Airflow 3.

It's also explained in this discussion: #37075 (comment), maybe someone will find it useful, so just leaving it here.

@potiuk

Copy link
Copy Markdown
Member

It's also explained in this discussion: #37075 (comment), maybe someone will find it useful, so just leaving it here.

Yep. We've been discussing this :)

@moiseenkov

moiseenkov commented Aug 21, 2024

Copy link
Copy Markdown
ContributorAuthor

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator:

@deprecated(reason="Please use `create_build_without_waiting_for_result`",removed_after="2025-03-01",category=AirflowProviderDeprecationWarning, )

That way the message can be generated programaticaly and be consistent that way -- no need for a precommit check then?

This is the perfect approach, and I'd be happy to apply it, but as it was discussed in #37075 (comment), implementing this new decorator on the Airflow core side would make it very difficult to use in providers as they would have to bump Airflow version in their requirements.

The idea of releasing such a decorator in a separated provider package seems like a great solution! Is there any chance to implement it any time soon? I see that this work has already started #36952. @kacpermuda , @potiuk

Otherwise, our team would be happy to merge the current PR (with date format fixed) now in order to bring some order to the google provider package now. And once the new provider with the structured deprecation decorator is released, we could adopt the current changes accordingly.

@potiuk

Copy link
Copy Markdown
Member

The idea of releasing such a decorator in a separated provider package seems like a great solution! Is there any chance to implement it any time soon? I see that this work has already started #36952. @kacpermuda , @potiuk

The common.compat provider is already there. It's really a matter of:

  • adding the decorator there (with a note which minimum versio of compat provider has it).
  • bumping minor version of the compat provider
  • add a dependency in google provider to common.compat>=NEW_VERSION
  • any other provider that will want to use it, will have to do the same

We could also add this decorator to "airflow" utils with the note that it will be usable when min airflow in providers will be >= 3.0 (but then it should be added in airlfow-sdk (cc: @ashb) - so we should not do it now, but possibly leave a TODO to add it there.

@kacpermuda

Copy link
Copy Markdown
Collaborator

The common.compat provider is already there.

My main concern, though I’m not sure if it’s entirely valid, is that we previously discussed having a common.utils provider specifically designed to make it easier to add simple functionality, without external dependencies, to all providers, so that they do not have to rely on core Airflow version. The common.compat provider, as I understand, is meant to serve as a “proxy” that ensures code within providers functions without errors, with potentially fewer features, regardless of the Airflow version or other provider versions, but we should gradually remove things from there as we raise min. dependency versions f.e. for Airflow core. So it looks similar, but I'm not quite sure it's the same. Introducing this type of deprecation module into common.compat could make it more multi-functional, and it can be beneficial, I'm just not sure if i understand the background logic behind common.compat

@potiuk

potiuk commented Aug 21, 2024

Copy link
Copy Markdown
Member

So it looks similar, but I'm not quite sure it's the same. Introducing this type of deprecation module into common.compat could make it more multi-functional, and it can be beneficial, I'm just not sure if i understand the background logic behind common.compat

It is the same. I imagine (see above) that the decorator WILL become part of "airflow's sdk" eventually (whether it will be named as such or "task.sdk" - generally in Airflow 3 this SDK will become the "util" equivalent - and this SDK will contain everything that provider should be able to use (and in Airflow 3 providers will have no dependency on Airflow "core" - they will have dependency on the "sdk". So in this case "compat" is precisely compatibilty shim for the future "common" SDK that airlfow providers will be able to use.

The only problem is that we do not YET have the SDK - this is being worked on as part of AIP-72 and will likely come together with restructuring of whole Airlfow repo - including moving providers to separate sub-projects and so on, so it makes very little sense to implement it now in airflow 3, because it will anyhow change.

@ashb - do you agree with that assessment ?

And note for the future - since you proposed it here - we have to be aware that any kind of the common features that are supposed to be used in providers will have to eventually land in "airflow.sdk" that should be always future-compatible and for now they should land also in "common.compat" if we want to implement it in current providers, since we want the providers to work for both Airlfow 2/3.

Does it make sense what I described here? @ashb - is this your understanding as well?

@moiseenkov

Copy link
Copy Markdown
ContributorAuthor

@kacpermuda , hi,
Considering the conversation here it seems that the best way for us is promoting your implementation #36952 (as it is almost complete) and release it in the common.compat provider package.
Would you have time for re-opening your PR and adding support for sunset dates?

@shahar1shahar1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent idea!
I'm OK with merging it in its current form after resolving the dates formatting.
If you have time to adjust it to the common.compat format as suggested - you're welcome to tag me in the new PR and I'll be happy to review it.

@kacpermuda

Copy link
Copy Markdown
Collaborator

@moiseenkov, I’m happy to revisit #36952, but unfortunately, I can’t commit to a specific timeline at the moment. If you have any available resources, feel free to pick it up in the meantime. Regardless, great job with this PR and moving us closer to more standardized deprecations! :)

@potiuk

Copy link
Copy Markdown
Member

@moiseenkov, I’m happy to revisit #36952, but unfortunately, I can’t commit to a specific timeline at the moment. If you have any available resources, feel free to pick it up in the meantime. Regardless, great job with this PR and moving us closer to more standardized deprecations! :)

I think we can also make it in stages.

Stage 1. Implement it in google provider (with google-specific deprecated decorator - with time and pre-commit checks.
Stage 2. Learn from it and move it (possibly extending this to other cases like version mentioned by @dstandish ) -> either to common.compat or future airflow-sdk if it's ready and depended on by providers by that time.

Google provider is big enough to start "some" standardization efforts - many cases and likely > 50% of all operators and there is a history of things started there and adopted by others and made "common" (system tests for example)

I don't think we should aim to have a final solution implemented from day 0 that is applicable to all providers - having google as a good start and "experiment" is IMHO - totally acceptable.

WDYT others?

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch 2 times, most recently from 0794bde to fe269c0CompareAugust 26, 2024 10:19
@moiseenkov

moiseenkov commented Aug 26, 2024

Copy link
Copy Markdown
ContributorAuthor

Hi everyone,
As we agreed, I introduced a custom @deprecated decorator within the Google provider package with a detailed structure and pre-commit hook that enforces a sunset date for changes within the provider. The date format is also adjusted.

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from fe269c0 to c52f944CompareAugust 26, 2024 10:43
Comment threadairflow/providers/google/common/deprecated.py Outdated
@potiuk
potiukforce-pushed the deprecation_eol_precommit branch from c52f944 to f0418c4CompareAugust 29, 2024 15:03
@VladaZakharova

Copy link
Copy Markdown
Contributor

Hi @potiuk@eladkal@ashb !
I think we can try to merge this,m WDYT? The longer we wait, the more information about those deprecations become irrelevant haha

@potiuk

Copy link
Copy Markdown
Member

I am all for it @eladkal@dstandish - I think you had doubts - but I think we can do it as 'google provider specific' for now. I guess small, provider-specific solutions like that are better than inaction and waiting for responses. We can always change it later - no harm done to any other providers.

Will merge if I don't hear a complain

@shahar1shahar1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from f0418c4 to c941b57CompareAugust 30, 2024 14:23
@potiuk
potiuk merged commit 86e3d29 into apache:mainSep 1, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-toolsarea:providersarea:secretsprovider:cncf-kubernetesKubernetes (k8s) provider related issuesprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@moiseenkov@potiuk@ashb@kacpermuda@VladaZakharova@dstandish@eladkal@shahar1
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Enforce deprecation message format with EOL for google provider package by moiseenkov · Pull Request #41637 · apache/airflow · GitHub
Skip to content

Enforce deprecation message format with EOL for google provider package - #41637

Merged
potiuk merged 2 commits into
apache:mainfrom
VladaZakharova:deprecation_eol_precommit
Sep 1, 2024
Merged

Enforce deprecation message format with EOL for google provider package#41637
potiuk merged 2 commits into
apache:mainfrom
VladaZakharova:deprecation_eol_precommit

Conversation

@moiseenkov

Copy link
Copy Markdown
Contributor

This PR introduces unification for deprecation messages formatting within Google's provider package by following:

  1. Updated the pre-commit hook check-code-deprecations so it enforces a specific message structure containing the end of life date for the deprecated entity. According to the deprecation policy discussed here, this deadline doesn't enforce removal of the deprecated entity from the source code, it only increases transparency and predictability for users, so they knew how much time do they have for the changes on their side.
  2. Another reason for this change is preparation for the cleaning-up process as Google's provider package has collected a lot of old legacy code and we'd be happy to finally clean it up.
  3. These changes work only within an airflow/providers/google scope.
  4. The default EOL is at least six months ahead.
  5. Some classes and methods have assigned earlier deadlines because they have been deprecated for a very long time already and can be removed earlier or their API is shut down and they are not working already.

@boring-cyborgboring-cyborgBot added area:dev-tools area:providers area:secrets provider:cncf-kubernetes Kubernetes (k8s) provider related issues provider:google Google (including GCP) related issues labels Aug 21, 2024
@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from 5e43f9e to bd97094CompareAugust 21, 2024 10:37

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NICE! I like it a lot.

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from bd97094 to 8f471c1CompareAugust 21, 2024 10:39
@potiuk

Copy link
Copy Markdown
Member

cc: @dstandish -> You wanted to do something like that - I think the approach proposed by the Google team (for now in google provider is really nice and we could apply it everywhere)

@ashb

ashb commented Aug 21, 2024

Copy link
Copy Markdown
Member

We should use yyyy-mm-dd (or yyyy.mm.dd, don't mind separator) so the date is unabigious

Comment threadairflow/providers/google/cloud/hooks/kubernetes_engine.py Outdated
@ashb

ashb commented Aug 21, 2024

Copy link
Copy Markdown
Member

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator:

@deprecated(reason="Please use `create_build_without_waiting_for_result`",removed_after="2025-03-01",category=AirflowProviderDeprecationWarning, )

That way the message can be generated programaticaly and be consistent that way -- no need for a precommit check then?

@kacpermuda

Copy link
Copy Markdown
Collaborator

Any standarization to deprecations is a good idea 🚀

did you consider adding extra (required) args to the deprecated decorator

Something related and still on my todo list (for whenever i have time) is #36952 where i wanted to add a custom decorators (or other construct) for different deprecations (function, arg, arg_value etc.), where we could separate these specific parts (what is deprecated, since when, removal date, what to use instead etc.) into separate arguments so it's easier to generate docs out of it. I think it could work even better now with common.compat provider, maybe we could implement something more global for the airflow codebase 😄

@potiuk

potiuk commented Aug 21, 2024

Copy link
Copy Markdown
Member

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator

Pre-commit check is still needed regardless - because we want to make sure that all deprecations have it. But yes - they could be better structured. one problem with adding new fields - is that since providers and airflow core are separated, anything "common" we come up with will have to go to "common.compat" - especially in the light that providers should be compatible with both Airflow 2 and Airflow 3.

So generally - yes better structure at the expense of more cross-provider dependencies.

@kacpermuda

Copy link
Copy Markdown
Collaborator

is that since providers and airflow core are separated, anything "common" we come up with will have to go to "common.compat" - especially in the light that providers should be compatible with both Airflow 2 and Airflow 3.

It's also explained in this discussion: #37075 (comment), maybe someone will find it useful, so just leaving it here.

@potiuk

Copy link
Copy Markdown
Member

It's also explained in this discussion: #37075 (comment), maybe someone will find it useful, so just leaving it here.

Yep. We've been discussing this :)

@moiseenkov

moiseenkov commented Aug 21, 2024

Copy link
Copy Markdown
ContributorAuthor

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator:

@deprecated(reason="Please use `create_build_without_waiting_for_result`",removed_after="2025-03-01",category=AirflowProviderDeprecationWarning, )

That way the message can be generated programaticaly and be consistent that way -- no need for a precommit check then?

This is the perfect approach, and I'd be happy to apply it, but as it was discussed in #37075 (comment), implementing this new decorator on the Airflow core side would make it very difficult to use in providers as they would have to bump Airflow version in their requirements.

The idea of releasing such a decorator in a separated provider package seems like a great solution! Is there any chance to implement it any time soon? I see that this work has already started #36952. @kacpermuda , @potiuk

Otherwise, our team would be happy to merge the current PR (with date format fixed) now in order to bring some order to the google provider package now. And once the new provider with the structured deprecation decorator is released, we could adopt the current changes accordingly.

@potiuk

Copy link
Copy Markdown
Member

The idea of releasing such a decorator in a separated provider package seems like a great solution! Is there any chance to implement it any time soon? I see that this work has already started #36952. @kacpermuda , @potiuk

The common.compat provider is already there. It's really a matter of:

  • adding the decorator there (with a note which minimum versio of compat provider has it).
  • bumping minor version of the compat provider
  • add a dependency in google provider to common.compat>=NEW_VERSION
  • any other provider that will want to use it, will have to do the same

We could also add this decorator to "airflow" utils with the note that it will be usable when min airflow in providers will be >= 3.0 (but then it should be added in airlfow-sdk (cc: @ashb) - so we should not do it now, but possibly leave a TODO to add it there.

@kacpermuda

Copy link
Copy Markdown
Collaborator

The common.compat provider is already there.

My main concern, though I’m not sure if it’s entirely valid, is that we previously discussed having a common.utils provider specifically designed to make it easier to add simple functionality, without external dependencies, to all providers, so that they do not have to rely on core Airflow version. The common.compat provider, as I understand, is meant to serve as a “proxy” that ensures code within providers functions without errors, with potentially fewer features, regardless of the Airflow version or other provider versions, but we should gradually remove things from there as we raise min. dependency versions f.e. for Airflow core. So it looks similar, but I'm not quite sure it's the same. Introducing this type of deprecation module into common.compat could make it more multi-functional, and it can be beneficial, I'm just not sure if i understand the background logic behind common.compat

@potiuk

potiuk commented Aug 21, 2024

Copy link
Copy Markdown
Member

So it looks similar, but I'm not quite sure it's the same. Introducing this type of deprecation module into common.compat could make it more multi-functional, and it can be beneficial, I'm just not sure if i understand the background logic behind common.compat

It is the same. I imagine (see above) that the decorator WILL become part of "airflow's sdk" eventually (whether it will be named as such or "task.sdk" - generally in Airflow 3 this SDK will become the "util" equivalent - and this SDK will contain everything that provider should be able to use (and in Airflow 3 providers will have no dependency on Airflow "core" - they will have dependency on the "sdk". So in this case "compat" is precisely compatibilty shim for the future "common" SDK that airlfow providers will be able to use.

The only problem is that we do not YET have the SDK - this is being worked on as part of AIP-72 and will likely come together with restructuring of whole Airlfow repo - including moving providers to separate sub-projects and so on, so it makes very little sense to implement it now in airflow 3, because it will anyhow change.

@ashb - do you agree with that assessment ?

And note for the future - since you proposed it here - we have to be aware that any kind of the common features that are supposed to be used in providers will have to eventually land in "airflow.sdk" that should be always future-compatible and for now they should land also in "common.compat" if we want to implement it in current providers, since we want the providers to work for both Airlfow 2/3.

Does it make sense what I described here? @ashb - is this your understanding as well?

@moiseenkov

Copy link
Copy Markdown
ContributorAuthor

@kacpermuda , hi,
Considering the conversation here it seems that the best way for us is promoting your implementation #36952 (as it is almost complete) and release it in the common.compat provider package.
Would you have time for re-opening your PR and adding support for sunset dates?

@shahar1shahar1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent idea!
I'm OK with merging it in its current form after resolving the dates formatting.
If you have time to adjust it to the common.compat format as suggested - you're welcome to tag me in the new PR and I'll be happy to review it.

@kacpermuda

Copy link
Copy Markdown
Collaborator

@moiseenkov, I’m happy to revisit #36952, but unfortunately, I can’t commit to a specific timeline at the moment. If you have any available resources, feel free to pick it up in the meantime. Regardless, great job with this PR and moving us closer to more standardized deprecations! :)

@potiuk

Copy link
Copy Markdown
Member

@moiseenkov, I’m happy to revisit #36952, but unfortunately, I can’t commit to a specific timeline at the moment. If you have any available resources, feel free to pick it up in the meantime. Regardless, great job with this PR and moving us closer to more standardized deprecations! :)

I think we can also make it in stages.

Stage 1. Implement it in google provider (with google-specific deprecated decorator - with time and pre-commit checks.
Stage 2. Learn from it and move it (possibly extending this to other cases like version mentioned by @dstandish ) -> either to common.compat or future airflow-sdk if it's ready and depended on by providers by that time.

Google provider is big enough to start "some" standardization efforts - many cases and likely > 50% of all operators and there is a history of things started there and adopted by others and made "common" (system tests for example)

I don't think we should aim to have a final solution implemented from day 0 that is applicable to all providers - having google as a good start and "experiment" is IMHO - totally acceptable.

WDYT others?

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch 2 times, most recently from 0794bde to fe269c0CompareAugust 26, 2024 10:19
@moiseenkov

moiseenkov commented Aug 26, 2024

Copy link
Copy Markdown
ContributorAuthor

Hi everyone,
As we agreed, I introduced a custom @deprecated decorator within the Google provider package with a detailed structure and pre-commit hook that enforces a sunset date for changes within the provider. The date format is also adjusted.

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from fe269c0 to c52f944CompareAugust 26, 2024 10:43
Comment threadairflow/providers/google/common/deprecated.py Outdated
@potiuk
potiukforce-pushed the deprecation_eol_precommit branch from c52f944 to f0418c4CompareAugust 29, 2024 15:03
@VladaZakharova

Copy link
Copy Markdown
Contributor

Hi @potiuk@eladkal@ashb !
I think we can try to merge this,m WDYT? The longer we wait, the more information about those deprecations become irrelevant haha

@potiuk

Copy link
Copy Markdown
Member

I am all for it @eladkal@dstandish - I think you had doubts - but I think we can do it as 'google provider specific' for now. I guess small, provider-specific solutions like that are better than inaction and waiting for responses. We can always change it later - no harm done to any other providers.

Will merge if I don't hear a complain

@shahar1shahar1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from f0418c4 to c941b57CompareAugust 30, 2024 14:23
@potiuk
potiuk merged commit 86e3d29 into apache:mainSep 1, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-toolsarea:providersarea:secretsprovider:cncf-kubernetesKubernetes (k8s) provider related issuesprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@moiseenkov@potiuk@ashb@kacpermuda@VladaZakharova@dstandish@eladkal@shahar1
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); })(); Enforce deprecation message format with EOL for google provider package by moiseenkov · Pull Request #41637 · apache/airflow · GitHub
Skip to content

Enforce deprecation message format with EOL for google provider package - #41637

Merged
potiuk merged 2 commits into
apache:mainfrom
VladaZakharova:deprecation_eol_precommit
Sep 1, 2024
Merged

Enforce deprecation message format with EOL for google provider package#41637
potiuk merged 2 commits into
apache:mainfrom
VladaZakharova:deprecation_eol_precommit

Conversation

@moiseenkov

Copy link
Copy Markdown
Contributor

This PR introduces unification for deprecation messages formatting within Google's provider package by following:

  1. Updated the pre-commit hook check-code-deprecations so it enforces a specific message structure containing the end of life date for the deprecated entity. According to the deprecation policy discussed here, this deadline doesn't enforce removal of the deprecated entity from the source code, it only increases transparency and predictability for users, so they knew how much time do they have for the changes on their side.
  2. Another reason for this change is preparation for the cleaning-up process as Google's provider package has collected a lot of old legacy code and we'd be happy to finally clean it up.
  3. These changes work only within an airflow/providers/google scope.
  4. The default EOL is at least six months ahead.
  5. Some classes and methods have assigned earlier deadlines because they have been deprecated for a very long time already and can be removed earlier or their API is shut down and they are not working already.

@boring-cyborgboring-cyborgBot added area:dev-tools area:providers area:secrets provider:cncf-kubernetes Kubernetes (k8s) provider related issues provider:google Google (including GCP) related issues labels Aug 21, 2024
@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from 5e43f9e to bd97094CompareAugust 21, 2024 10:37

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NICE! I like it a lot.

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from bd97094 to 8f471c1CompareAugust 21, 2024 10:39
@potiuk

Copy link
Copy Markdown
Member

cc: @dstandish -> You wanted to do something like that - I think the approach proposed by the Google team (for now in google provider is really nice and we could apply it everywhere)

@ashb

ashb commented Aug 21, 2024

Copy link
Copy Markdown
Member

We should use yyyy-mm-dd (or yyyy.mm.dd, don't mind separator) so the date is unabigious

Comment threadairflow/providers/google/cloud/hooks/kubernetes_engine.py Outdated
@ashb

ashb commented Aug 21, 2024

Copy link
Copy Markdown
Member

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator:

@deprecated(reason="Please use `create_build_without_waiting_for_result`",removed_after="2025-03-01",category=AirflowProviderDeprecationWarning, )

That way the message can be generated programaticaly and be consistent that way -- no need for a precommit check then?

@kacpermuda

Copy link
Copy Markdown
Collaborator

Any standarization to deprecations is a good idea 🚀

did you consider adding extra (required) args to the deprecated decorator

Something related and still on my todo list (for whenever i have time) is #36952 where i wanted to add a custom decorators (or other construct) for different deprecations (function, arg, arg_value etc.), where we could separate these specific parts (what is deprecated, since when, removal date, what to use instead etc.) into separate arguments so it's easier to generate docs out of it. I think it could work even better now with common.compat provider, maybe we could implement something more global for the airflow codebase 😄

@potiuk

potiuk commented Aug 21, 2024

Copy link
Copy Markdown
Member

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator

Pre-commit check is still needed regardless - because we want to make sure that all deprecations have it. But yes - they could be better structured. one problem with adding new fields - is that since providers and airflow core are separated, anything "common" we come up with will have to go to "common.compat" - especially in the light that providers should be compatible with both Airflow 2 and Airflow 3.

So generally - yes better structure at the expense of more cross-provider dependencies.

@kacpermuda

Copy link
Copy Markdown
Collaborator

is that since providers and airflow core are separated, anything "common" we come up with will have to go to "common.compat" - especially in the light that providers should be compatible with both Airflow 2 and Airflow 3.

It's also explained in this discussion: #37075 (comment), maybe someone will find it useful, so just leaving it here.

@potiuk

Copy link
Copy Markdown
Member

It's also explained in this discussion: #37075 (comment), maybe someone will find it useful, so just leaving it here.

Yep. We've been discussing this :)

@moiseenkov

moiseenkov commented Aug 21, 2024

Copy link
Copy Markdown
ContributorAuthor

Instead of a pre-commit check and enforcing the style of the message did you consider adding extra (required) args to the deprecated decorator:

@deprecated(reason="Please use `create_build_without_waiting_for_result`",removed_after="2025-03-01",category=AirflowProviderDeprecationWarning, )

That way the message can be generated programaticaly and be consistent that way -- no need for a precommit check then?

This is the perfect approach, and I'd be happy to apply it, but as it was discussed in #37075 (comment), implementing this new decorator on the Airflow core side would make it very difficult to use in providers as they would have to bump Airflow version in their requirements.

The idea of releasing such a decorator in a separated provider package seems like a great solution! Is there any chance to implement it any time soon? I see that this work has already started #36952. @kacpermuda , @potiuk

Otherwise, our team would be happy to merge the current PR (with date format fixed) now in order to bring some order to the google provider package now. And once the new provider with the structured deprecation decorator is released, we could adopt the current changes accordingly.

@potiuk

Copy link
Copy Markdown
Member

The idea of releasing such a decorator in a separated provider package seems like a great solution! Is there any chance to implement it any time soon? I see that this work has already started #36952. @kacpermuda , @potiuk

The common.compat provider is already there. It's really a matter of:

  • adding the decorator there (with a note which minimum versio of compat provider has it).
  • bumping minor version of the compat provider
  • add a dependency in google provider to common.compat>=NEW_VERSION
  • any other provider that will want to use it, will have to do the same

We could also add this decorator to "airflow" utils with the note that it will be usable when min airflow in providers will be >= 3.0 (but then it should be added in airlfow-sdk (cc: @ashb) - so we should not do it now, but possibly leave a TODO to add it there.

@kacpermuda

Copy link
Copy Markdown
Collaborator

The common.compat provider is already there.

My main concern, though I’m not sure if it’s entirely valid, is that we previously discussed having a common.utils provider specifically designed to make it easier to add simple functionality, without external dependencies, to all providers, so that they do not have to rely on core Airflow version. The common.compat provider, as I understand, is meant to serve as a “proxy” that ensures code within providers functions without errors, with potentially fewer features, regardless of the Airflow version or other provider versions, but we should gradually remove things from there as we raise min. dependency versions f.e. for Airflow core. So it looks similar, but I'm not quite sure it's the same. Introducing this type of deprecation module into common.compat could make it more multi-functional, and it can be beneficial, I'm just not sure if i understand the background logic behind common.compat

@potiuk

potiuk commented Aug 21, 2024

Copy link
Copy Markdown
Member

So it looks similar, but I'm not quite sure it's the same. Introducing this type of deprecation module into common.compat could make it more multi-functional, and it can be beneficial, I'm just not sure if i understand the background logic behind common.compat

It is the same. I imagine (see above) that the decorator WILL become part of "airflow's sdk" eventually (whether it will be named as such or "task.sdk" - generally in Airflow 3 this SDK will become the "util" equivalent - and this SDK will contain everything that provider should be able to use (and in Airflow 3 providers will have no dependency on Airflow "core" - they will have dependency on the "sdk". So in this case "compat" is precisely compatibilty shim for the future "common" SDK that airlfow providers will be able to use.

The only problem is that we do not YET have the SDK - this is being worked on as part of AIP-72 and will likely come together with restructuring of whole Airlfow repo - including moving providers to separate sub-projects and so on, so it makes very little sense to implement it now in airflow 3, because it will anyhow change.

@ashb - do you agree with that assessment ?

And note for the future - since you proposed it here - we have to be aware that any kind of the common features that are supposed to be used in providers will have to eventually land in "airflow.sdk" that should be always future-compatible and for now they should land also in "common.compat" if we want to implement it in current providers, since we want the providers to work for both Airlfow 2/3.

Does it make sense what I described here? @ashb - is this your understanding as well?

@moiseenkov

Copy link
Copy Markdown
ContributorAuthor

@kacpermuda , hi,
Considering the conversation here it seems that the best way for us is promoting your implementation #36952 (as it is almost complete) and release it in the common.compat provider package.
Would you have time for re-opening your PR and adding support for sunset dates?

@shahar1shahar1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent idea!
I'm OK with merging it in its current form after resolving the dates formatting.
If you have time to adjust it to the common.compat format as suggested - you're welcome to tag me in the new PR and I'll be happy to review it.

@kacpermuda

Copy link
Copy Markdown
Collaborator

@moiseenkov, I’m happy to revisit #36952, but unfortunately, I can’t commit to a specific timeline at the moment. If you have any available resources, feel free to pick it up in the meantime. Regardless, great job with this PR and moving us closer to more standardized deprecations! :)

@potiuk

Copy link
Copy Markdown
Member

@moiseenkov, I’m happy to revisit #36952, but unfortunately, I can’t commit to a specific timeline at the moment. If you have any available resources, feel free to pick it up in the meantime. Regardless, great job with this PR and moving us closer to more standardized deprecations! :)

I think we can also make it in stages.

Stage 1. Implement it in google provider (with google-specific deprecated decorator - with time and pre-commit checks.
Stage 2. Learn from it and move it (possibly extending this to other cases like version mentioned by @dstandish ) -> either to common.compat or future airflow-sdk if it's ready and depended on by providers by that time.

Google provider is big enough to start "some" standardization efforts - many cases and likely > 50% of all operators and there is a history of things started there and adopted by others and made "common" (system tests for example)

I don't think we should aim to have a final solution implemented from day 0 that is applicable to all providers - having google as a good start and "experiment" is IMHO - totally acceptable.

WDYT others?

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch 2 times, most recently from 0794bde to fe269c0CompareAugust 26, 2024 10:19
@moiseenkov

moiseenkov commented Aug 26, 2024

Copy link
Copy Markdown
ContributorAuthor

Hi everyone,
As we agreed, I introduced a custom @deprecated decorator within the Google provider package with a detailed structure and pre-commit hook that enforces a sunset date for changes within the provider. The date format is also adjusted.

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from fe269c0 to c52f944CompareAugust 26, 2024 10:43
Comment threadairflow/providers/google/common/deprecated.py Outdated
@potiuk
potiukforce-pushed the deprecation_eol_precommit branch from c52f944 to f0418c4CompareAugust 29, 2024 15:03
@VladaZakharova

Copy link
Copy Markdown
Contributor

Hi @potiuk@eladkal@ashb !
I think we can try to merge this,m WDYT? The longer we wait, the more information about those deprecations become irrelevant haha

@potiuk

Copy link
Copy Markdown
Member

I am all for it @eladkal@dstandish - I think you had doubts - but I think we can do it as 'google provider specific' for now. I guess small, provider-specific solutions like that are better than inaction and waiting for responses. We can always change it later - no harm done to any other providers.

Will merge if I don't hear a complain

@shahar1shahar1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@moiseenkov
moiseenkovforce-pushed the deprecation_eol_precommit branch from f0418c4 to c941b57CompareAugust 30, 2024 14:23
@potiuk
potiuk merged commit 86e3d29 into apache:mainSep 1, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-toolsarea:providersarea:secretsprovider:cncf-kubernetesKubernetes (k8s) provider related issuesprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@moiseenkov@potiuk@ashb@kacpermuda@VladaZakharova@dstandish@eladkal@shahar1