-
Notifications
You must be signed in to change notification settings - Fork 14.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove pid arg from celery option to fix duplicate pid issue, Move celery command to provider package #36794
Conversation
Tests failing |
@potiuk can you tell me why its failing to import module from airflow, core modules should be available to providers right
|
It's failing in compatibility check. When you install the provider in Airflow 2.6 or 2.7, the module is not there because it has been added later. That's whe we have those compatibility checks - they install the provider in Airflow 2.6 and 2.7 and attempt to import all classes (and fail). You need make sure to handle back-compatibility here. Likely you should in-line this method with a comment that it should be removed when min_airflow_version is 2.8.0 we have few other cases like that in various providers. |
@potiuk I tried copying daemon func but it goes way down where so many utils are not present. Its better if someone with better codebase understanding do this. Let me know if I should create new PR with fix in core cli |
Actually I realized there is a simple way you can get back to what it was before. This command in Provider will be only useful since Airflow 2.9 so you can do something similar, when importing the daemon_utils to what is done in auth manager: try:
from airflow.auth.managers.base_auth_manager import BaseAuthManager, ResourceMethod
except ImportError:
raise AirflowOptionalProviderFeatureException(
"Failed to import BaseUser. This feature is only available in Airflow versions >= 2.8.0"
) |
So simply go back to previous import and add the try/except/raise exception and all should be good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @o-nikolas ?
WDYT @o-nikolas ? |
So we went with deprecation messages instead of the approach of automating the action of keeping the two modules in sync via pre-commit? I'm still worried there will be drift but I suppose adding the precommit stuff is a bit tricky so I'd be okay with this being merged. But I do agree with @potiuk's suggestion above. |
Celery provider has an ambedded Airflow CLI command as of 3.6.1. When the apache#36794 was merged, we thought mistakenly that it will only be used in airflow 2.9.0+, so we used a feature introduced in Airflow 2.8.0 in the apache#34945 - but in fact the CLI command is configured by the Celery Executor which is also part of the Celery provider, so it was also used for airflow < 2.8.0 and failed due to missing import. This PR checks if Airflow version is < 2.8.0 and if so, it falls back to built-in airflow CLI command.
Celery provider has an ambedded Airflow CLI command as of 3.6.1. When the #36794 was merged, we thought mistakenly that it will only be used in airflow 2.9.0+, so we used a feature introduced in Airflow 2.8.0 in the #34945 - but in fact the CLI command is configured by the Celery Executor which is also part of the Celery provider, so it was also used for airflow < 2.8.0 and failed due to missing import. This PR checks if Airflow version is < 2.8.0 and if so, it falls back to built-in airflow CLI command.
closes: #36541