Move fs scheme definition from Python to YAML

[uranusjr]

I recall someone (is it @ashb?) mentioning somewhere else most provider-supplied features are defined in provider.yml instead of in Python code.

[bolkedebruin]

LGTM, but will require a release of the providers cc @eladkal, preferably before 2.8 I think? And this should then also be in 2.8 in order not to provide backwards compatibility? cc @ephraimbuddy

[potiuk]

Maybe a new item "fsspec" as a key instead of "filesystems"?

[uranusjr]

It’s totally pratical for the plugin manager to just support the old format since it is very trivial, even indefinitely, as long as we don’t put it in the schema. Since we have not released a stable Airflow version using the old format, it would take very little code to cover both cases. Does that sound good enough?

[potiuk]

It’s totally pratical for the plugin manager to just support the old format since it is very trivial, even indefinitely, as long as we don’t put it in the schema. Since we have not released a stable Airflow version using the old format, it would take very little code to cover both cases. Does that sound good enough?

Yep. Good enough

[uranusjr]

Compatibility added to load the old format, with a test.

[potiuk]

The test needs to be fixed

[uranusjr]

Since 2.8.0 is released, all provider.yml updates are now blocked until the affected providers require 2.8+. This is suboptimal.

[potiuk]

What do you mean by blocked ?

[uranusjr]

Since 2.8.0 does not contain logic to handle the new format, a provider cannot change its provider.yml until it drops support to the version.

[potiuk]

Since 2.8.0 does not contain logic to handle the new format, a provider cannot change its provider.yml until it drops support to the version.

Well. Provider.yaml and Provider.info had been (since 2.0.1 where we defined the schema, 2.0.0 indeed had the problem you mentioned) built for extensibility - it means that you can add any extra elements to provider.yaml - they are verified at main via validating the schema and they are validated by earlier versions of Airflow via earlier provider schema, but this schema allows to add extra elements that get ignored. So it's perfectly fine to add new elements to provider.yaml (and they get exposed via get_provider_info ) as they will be simply ignored by earlier versions of Airflow.

Similarly if a provider adds a new functionality that should only be used in later version of Airflow (say exposing FS interface), we have the mechanism of throwing "AirflowOptionalProviderFeatureException" during importing of relevant modules (for example when there is missing import or where we simply recognize that this module gets imported in the environment where it should not be - for example by checking airflow version). This is the way how you can add features from "future" versions of Airlfow even even your provider still supports 2.6+

I believe these two mechanisms solve whatever compatibility problems there might be and they do not "block" any changes in provider.yaml.

Or are you talking about something else? Maybe there is a bug or a scenario that is not handled well?

Could you please give an example of where are you blocked?

[potiuk]

And just add a bit - it would be quite surprising to see somethig "blocking" provider.yaml changes - we've been adding similar features to provider.yaml in pretty much every single MINOR version of Airflow and it's never been blocking anyone from anything (except 2.0.0 where we recognized it and documented that 2.0.0 will not work with future providers because it lack of flexibility) - it's been even documented in release notes.

[potiuk]

Ah, i see now. Yes you are right. The fact that we released Airflow 2.8.0 without support for this schema means that providers will have to expose both interfaces. Unitl they have 2.9.0.

I am afraid it's too late. Yes - this is why we have voting and release process - if you would have -1 one of the RCs during the voting, we would have probably changed, it, but you didn't, so we will have to live with it. Another option is to fix it in 2.8.1 as a bug-fix and yank 2.8.0. I think this shoudl be our learning that we have the power to stop the release during voting if we see things like that.

[potiuk]

Maybe @uranusjr - raise this point at the devlist as usual?

[potiuk]

BTW. We are not blocked "really" from changing provider.yaml. simply we have to expose (and handle) both interfaces.

[bolkedebruin]

Mmm yes. This feels a bit like XML config to me (so the pattern I mean, the separation of what the code can do and how it is figured out what it can and you need to look at several places to make things work) yanking seems to best option?

Sorrry @uranusjr missing this for 2.8 btw.

[potiuk]

Just one watchout - It's a bit of tough decision though - we rarely (I can't even remember it) yank airflow version and we need to have good reason for it. I personally don't have anything against yanking a version of our package, but for many people it might be sign of we have not done a good job there - as if admiting a mistake and fixing it is wrong.

I personally think there is nothing wrong in saying yeah, bad thing we've done we should correct it now. So I think this should be made into a devlist discussion, because I think there will be people who will be surprised that we are yanking airflow.

[eladkal]

and yank 2.8.0.

I dont think it's right to yank 2.8.0 over this. I am OK to habdle it as bug fix but this means 2.8.1 should be created this week to reduce the effect on users.

[potiuk]

Yeah. yanking is a bit extreme measure, I agree.

[bolkedebruin]

We did mark AFS as experimental right? So, in theory we could be a bit more daring in not doing backwards compatibility?

[potiuk]

We did mark AFS as experimental right? So, in theory we could be a bit more daring in not doing backwards compatibility?

Well. Not if we are targetting to break our own, released providers .. That's like introducing a deliberate breaking change that breaks what's already released there.

I think we have to be really careful when we are pushing out a chang that our providers depend on. Yes, it's more effort and yes, keeping the backwards compatibility will cost us, but I'd say if we want to release a code that our provider depends on, then we should NOT release it if we see backwards compatibility problems in sighte. We could use feature flags from AIP-44 (internal API) for example now and AIP-52 (Setup/Terdown) had been doing that in the past - we released new Airflow version with those "partially done" but disabled.

We could have done it this time as well if we felt that current implementation is not ready for prime time (and if felt we are not ready to support backwards compatibility).

I think in this case we shoudl simply acknowledge we were rushing too fast, bite the bullet, and introduce and maintain compatibility layer. - treating it as valuable lesson to be more careful wiith such changes in the future.

[bolkedebruin]

Throwing the cat among the pigeons, if we are to have this compatibility layer indefinitely should we then maybe just stick with the initial implementation?

[potiuk]

Throwing the cat among the pigeons, if we are to have this compatibility layer indefinitely should we then maybe just stick with the initial implementation?

I personally am in favour of continuously improving and standardizing things even if it means slight back-compatibility maintenance effort.

If it is really just few lines of back-compatibility that cost us none or little, I see no problem with keeping it if we have a chance to apply it accros all the future codebase and make it cleaner, more straightforward and easier to read.

We have a precendent, and datapoints that we can use so this is not an academic discussion.

We already did that for Provider's Hook class discovery. At one point in time we started to support new way of discovering Hook classes (the main reason there was performance of discovery, we do not have to import and istantiate Hook classes to find out what connection types they support.

What happened:

• original code was implemented 29 Nov 2020

• new way of discovering Hook classes was implementd 19 Aug 2021 (10 months later)

• Original method here: https://github.com/apache/airflow/blob/main/airflow/providers_manager.py#L751

• New method here: https://github.com/apache/airflow/blob/main/airflow/providers_manager.py#L705

• we paid virtually no maintenance cost - it's virtually that one method and the only changes to it were bulk changes applied and errors suppression: The only "real" change was https://github.com/apache/airflow/commit/b5a786b38148295c492da8ab731d5e2f6f86ccf7 by Daniel which was small refactor applied to both methods and error suppression.

• it's covered by test cases so it still works.

• we have a code that we had been barely touched for ~ 2 years

• all our providers have nicer, more performant and cleaner interface

• I actually completely forgot about this part of code - this discussion made me remember it and look at that code for first time for maybe 1.5 year

This is a complete list of changes to that code:

I personally see no reason why this case would be different.

We probably already spent more time on discussing consequences of it than we will ever spend on maintaining it.

[github-actions[bot]]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

[potiuk]

Are you planning to continue that one @uranusjr / @bolkedebruin ?

[github-actions[bot]]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.