Fix compatibility of updated airflow.io with released providers
#36186
Conversation
The released providers added support to previous version of the `airflow.io` - where options were not passed to `get_fs` method that provides Fsspec compatible FileSystem. However apache#35820 added positional "options" parameter when the method is called and it broke already released providers. This PR dynamically inspects signature of the get_fs method and when one parameter is detected, it will skip passing options to get_fs method call.
| parameters = inspect.signature(fs).parameters | ||
| if len(parameters) == 1: | ||
| return fs(conn_id) # type: ignore[call-arg] | ||
| return fs(conn_id, options) # type: ignore[call-arg] |
There was a problem hiding this comment.
Should we add a test for the old signature to verify
There was a problem hiding this comment.
Let me see. It's new code for me :)
There was a problem hiding this comment.
Looks like there are no tests even for retrieval for providers, if time is of an essence, I'd rather leave it for later. created an issue: #36187
There was a problem hiding this comment.
Isn't the typical pythonic way to do this with a try catch? Inspect is expensive.
There was a problem hiding this comment.
yeah - it's a bit more expensive, but It happens only once at discovery.
I thought about it too but I decided dointg it more explicitly is better. It is much better to handle (potential) patterns where we add more parameters (which might happen). In this case we will just add elif len() ==2 - it would be difficult to distinguish those cases with exception.
There was a problem hiding this comment.
Also, this is pretty common pattern in Airflow already. We use inspect and looking at parameters and comparing signatures of methods in quite a numer of places already. I personally see it as much more explicit. But of course nothing prevents us from changing it in the future if we agree it's a better solution, this part is internal and can be changed any time.
ephraimbuddy
added
changelog:skip
…6186) The released providers added support to previous version of the `airflow.io` - where options were not passed to `get_fs` method that provides Fsspec compatible FileSystem. However #35820 added positional "options" parameter when the method is called and it broke already released providers. This PR dynamically inspects signature of the get_fs method and when one parameter is detected, it will skip passing options to get_fs method call. (cherry picked from commit 7799c51)
|
Mmm yes I understand. Sorry for this, I mentioned when merging the other it would require a release of the providers and wanted to prevent backwards compat check (which is silly imho as the current set of providers cannot work). |
3 participants
The released providers added support to previous version of the
airflow.io- where options were not passed toget_fsmethod that provides Fsspec compatible FileSystem. However #35820 added positional "options" parameter when the method is called and it broke already released providers.This PR dynamically inspects signature of the get_fs method and when one parameter is detected, it will skip passing options to get_fs method call.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.