dabla commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2050247170
> > But if you confirm this is fine then no problem I will do it like this :) > > Yes. We do it in a number of other places - you already looked at it. > > > couldn't we adapt the verify_providers module as that is the one that causing the import issue and check that if the imported module isn't an ignored providers from BASE_PROVIDERS_COMPATIBILITY_CHECKS? > > Probably in this particular case it would work - but there are other imports that are ignored for similar reasons - not only for "transfer" operators. Example from aws security manager: > > ``` > try: > from airflow.www.security_manager import AirflowSecurityManagerV2 > except ImportError: > raise AirflowOptionalProviderFeatureException( > "Failed to import AirflowSecurityManagerV2. This feature is only available in Airflow versions >= 2.8.0" > ) > ``` > > So the "optional feaature" is used for more things and here we can use it easily. > > Adding logic to handle import for excluded provider might add even more complexity and needs someone to do it :) - but yes if you would like to do it now for example that woudl work, you'd just need to pass the excluded providers to the inside of the container image where the providers built are installed in. The providers are imported inside the image that has all 690+ dependencies installed, because otherwise a lot of them fail, so the import is done inside Breeze image, and it adds a bit of complexity. Adding three imports sounds easier (and likely can take a lot less time). But if you would like to do it - go for it, I just feel you already spent a lot of time on this one :) > > Some more comments: > > * using local imports is one thing that we want to avoid - because this is where compatibility checks will stop detecting errors altogether. and in your example you could even simply not wrap it in optionalfeature exception. In this case we **know** where the error comes from so we can add it. > > > with optional_provider_feature(): > > Surely, but we have the same problem with common code. If we add it today, it's only going to be really usable 10 months from now. because providers are importing stuff from earlier airflow versions that won't have it. I will do it the quick and fast way as you showed for now so that we can close this one. Yeah that's what I thought afterwards also, the dependency issue. Could be nice though, could even have a decorator for this which would make it even nicer, will see... -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
