Auto-apply apply_default decorator #15667
Conversation
boring-cyborg
bot
added
area:core-operators
|
The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason. |
|
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
|
The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason. |
|
The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason. |
There was a problem hiding this comment.
As discussed on slack. We cannot do that, because we continue releasing providers from master, so the failure of that tests is precisely what should happen.
We have the following options:
- Wait with merging it to just before we cut the branch for 2.1 release (i.e. before we release 2.1rc) - with ~2 weeks release timeline
- Have a separate branch for providers-only release where we could cherry-pick provider's changes
- Refactor code automatically for provider release process.
I think option 1) sounds like easiest to do (and it has very little drawbacks, the PR will simply have to be rebased periodically + removal of a new @apply_defaults.
|
That's why I'm picking that this now, as it's what we are days away 2.1 release, and I was thinking that the small window of not being able to release providers was okay. |
|
Yeah. If it's 'days' then we can do option 2) cut the branch where we can cherry-pick any provider changes where we could relase providers from by cherry-picking if needed - just before merging this one, and yeah, in this case we can disable the "2.0" build and enable it back pointing to 2.1.0 right after the rc1 for 2.1.0 gets released. We'd then have to add but I think it would be OK to do it when we are sure rc1 is going to be released "shortly" :) |
ashb
force-pushed
the
AIRFLOW-6829-metaclass
branch
4 times, most recently
from
c7e915e to
04f072a
Compare
|
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
|
The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason. |
This makes two changes to make the apply_defaults transparent to warnings: 1. It removes `__call__` from the Metaclass (which added one frame) 2. It "patches" warnings.warn _in the function scope_ to automatically adjust the stacklevel The reason for this is that now that apply_defaults is called "magically" from the Metaclass we're adding extra frames to the callstack. By locally patching warnings like this it means that user code doesn't have to be aware of what we've done. (I wish Python had a native way of handling this case as it crops up all the time. Perhaps I should submit a PEP for it with a better design)
Since we have changed the dep to need Airflow 2.1, testing against 2.0 no longer makes any sense. And since 2.1 is not yet out, we have disabled this specific step for now
ashb
force-pushed
the
AIRFLOW-6829-metaclass
branch
from
20f9c11 to
f375d07
Compare
|
@potiuk Happy to merge this now? |
|
Absolutely. I will branch-off any changes in providers in case we need to release them with 2.0 compatibility. |
The GCSToLocalFilesystemOperator in Google Provider <=3.0.0 had wrong import for apply_defaults. It used `from airflow.sensors.base_sensor_operator import apply_defaults` instead of `from airflow.utils.decorators import apply_defaults` When we removed apply_defaults in apache#15667, the base_sensor_operator import was removed as well which made the GCSToLocalFilestystemOperator stops working in 2.1.0 Fixes: apache#16035
The GCSToLocalFilesystemOperator in Google Provider <=3.0.0 had wrong import for apply_defaults. It used `from airflow.sensors.base_sensor_operator import apply_defaults` instead of `from airflow.utils.decorators import apply_defaults` When we removed apply_defaults in #15667, the base_sensor_operator import was removed as well which made the GCSToLocalFilestystemOperator stops working in 2.1.0 Fixes: #16035
The GCSToLocalFilesystemOperator in Google Provider <=3.0.0 had wrong import for apply_defaults. It used `from airflow.sensors.base_sensor_operator import apply_defaults` instead of `from airflow.utils.decorators import apply_defaults` When we removed apply_defaults in apache#15667, the base_sensor_operator import was removed as well which made the GCSToLocalFilestystemOperator stops working in 2.1.0 Fixes: apache#16035 (cherry picked from commit 0f8f66e)
The GCSToLocalFilesystemOperator in Google Provider <=3.0.0 had wrong import for apply_defaults. It used `from airflow.sensors.base_sensor_operator import apply_defaults` instead of `from airflow.utils.decorators import apply_defaults` When we removed apply_defaults in #15667, the base_sensor_operator import was removed as well which made the GCSToLocalFilestystemOperator stops working in 2.1.0 Fixes: #16035 (cherry picked from commit 0f8f66e)
|
I noticed that @apply_defaults is deprecated but it was left with the following comment: "Make it still be a wrapper to keep the previous behaviour of an extra stack frame" It seems to me the number of scenarios where this will make a difference is relatively limited, but considering you chose to do it this way, I'm wondering if there are situations where a user cannot expect to simply remove the |
|
No |
Continuation of, and closes #15044
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.