Skip to content

Conversation

@dabla
Copy link
Contributor

@dabla dabla commented Aug 18, 2025

This is a fix for "An error occurred: You cannot use AsyncToSync in the same thread as an async event loop - just await the async function directly." in Airflow 3, as get_conn should be async.

closes: #54598


^ 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.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

…x "An error occurred: You cannot use AsyncToSync in the same thread as an async event loop - just await the async function directly." in Airflow 3
@gopidesupavan
Copy link
Member

@dabla thanks overall LGTM, some tests needs to be fixed.

BTW this is complete breaking change for AF2 users or anyone using these hooks methods , because get_conn is now async awaitable. so its better to add some compatibility and throw warning to users ?

Offcourse this change must for this operator to work with AF3.

@gopidesupavan
Copy link
Member

cc: @eladkal FYI.

dabla added 2 commits August 19, 2025 08:47
…ype and test_throw_failed_responses_with_application_json_content_type
@dabla
Copy link
Contributor Author

dabla commented Aug 19, 2025

@dabla thanks overall LGTM, some tests needs to be fixed.

BTW this is complete breaking change for AF2 users or anyone using these hooks methods , because get_conn is now async awaitable. so its better to add some compatibility and throw warning to users ?

Offcourse this change must for this operator to work with AF3.

We could but normally this is an "internal" method, as user would normally not interact with it when using operator of even the hook as there you have the run method. WDYT as get_conn is expected to be async now.

@eladkal
Copy link
Contributor

eladkal commented Aug 19, 2025

BTW this is complete breaking change for AF2 users or anyone using these hooks methods , because get_conn is now async awaitable. so its better to add some compatibility and throw warning to users ?

This is not good. I don't think we can break Airflow 2.10 comparability for this do we?

@dabla
Copy link
Contributor Author

dabla commented Aug 19, 2025

BTW this is complete breaking change for AF2 users or anyone using these hooks methods , because get_conn is now async awaitable. so its better to add some compatibility and throw warning to users ?

This is not good. I don't think we can break Airflow 2.10 comparability for this do we?

I don't think it's broken for Airflow 2 as this is "internal kitchen stuff", I can test it to make sure in Airflow 2.

@dabla
Copy link
Contributor Author

dabla commented Aug 19, 2025

BTW this is complete breaking change for AF2 users or anyone using these hooks methods , because get_conn is now async awaitable. so its better to add some compatibility and throw warning to users ?

This is not good. I don't think we can break Airflow 2.10 comparability for this do we?

If they directly interact with get_conn then yes it's would be broken, but that would be a bad practise normally you should use the run method. I can add a new get_async_conn method and keep the original for backward compatibility.

@dabla
Copy link
Contributor Author

dabla commented Aug 19, 2025

BTW this is complete breaking change for AF2 users or anyone using these hooks methods , because get_conn is now async awaitable. so its better to add some compatibility and throw warning to users ?

This is not good. I don't think we can break Airflow 2.10 comparability for this do we?

I'll made get_conn synced again and create new method get_async_conn and deprecate the former one.

@eladkal
Copy link
Contributor

eladkal commented Aug 19, 2025

If they directly interact with get_conn then yes it's would be broken, but that would be a bad practise normally you should use the run method. I can add a new get_async_conn method and keep the original for backward compatibility.

I'm not into the details of this change. If this works on AF2 and we have reasonable claim to classify it as bug fix then this is fine. If mitigation steps or special instructions into what was changed are needed then we need a log entry to specify this change (a key indication to know if this is required is: if we expect that as a result of releasing this change workflows will behave differently)

@dabla
Copy link
Contributor Author

dabla commented Aug 19, 2025

If they directly interact with get_conn then yes it's would be broken, but that would be a bad practise normally you should use the run method. I can add a new get_async_conn method and keep the original for backward compatibility.

I'm not into the details of this change. If this works on AF2 and we have reasonable claim to classify it as bug fix then this is fine. If mitigation steps or special instructions into what was changed are needed then we need a log entry to specify this change (a key indication to know if this is required is: if we expect that as a result of releasing this change workflows will behave differently)

Well I just kept original method and added new async one like other providers do, it wasn't big of a deal after all, and so we are certain we don't break anything, @gopidesupavan was right by mentioning this.

@dabla dabla marked this pull request as draft August 19, 2025 07:44
@eladkal
Copy link
Contributor

eladkal commented Aug 19, 2025

Well I just kept original method and added new async one like other providers do, it wasn't big of a deal after all, and so we are certain we don't break anything, @gopidesupavan was right by mentioning this.

Cool

@dabla
Copy link
Contributor Author

dabla commented Aug 19, 2025

Will adapt PowerBI related triggers also so it's aligned with MSGraph as they use the same code.

@dabla dabla marked this pull request as ready for review August 19, 2025 09:08
@dabla
Copy link
Contributor Author

dabla commented Aug 19, 2025

@eladkal Should be ready now

@dabla dabla requested a review from gopidesupavan August 19, 2025 12:50
@gopidesupavan
Copy link
Member

Thanks for the update, instead of removing the properties its better to throw depreciation warning. my only concern is all these are public facing methods or properties, not how users interact with these so its better with deprecation warning. WDYT?

@dabla
Copy link
Contributor Author

dabla commented Aug 20, 2025

Thanks for the update, instead of removing the properties its better to throw depreciation warning. my only concern is all these are public facing methods or properties, not how users interact with these so its better with deprecation warning. WDYT?

I've re-added get_conn with deprecation.

@dabla dabla requested a review from gopidesupavan August 21, 2025 12:09
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
cc @gopidesupavan

Copy link
Member

@gopidesupavan gopidesupavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely thanks @dabla :)

@gopidesupavan gopidesupavan merged commit 9c1f368 into apache:main Aug 21, 2025
75 checks passed
mangal-vairalkar pushed a commit to mangal-vairalkar/airflow that referenced this pull request Aug 30, 2025
…Sync error (apache#54598)

* refactor: Made get_conn method of KiotaRequestAdapterHook async to fix "An error occurred: You cannot use AsyncToSync in the same thread as an async event loop - just await the async function directly." in Airflow 3

* refactor: Fixed static checks

* refactor: Re-added missing import HttpxRequestAdapter

* refactor: Reformatted imports

* refactor: Fixed imports

* refactor: Added missing AirflowProviderDeprecationWarning

* refactor: Fixed test_serialize

* refactor: Fixed test_throw_failed_responses_with_text_plain_content_type and test_throw_failed_responses_with_application_json_content_type

* refactor: Renamed async get_conn method to get_async_conn in KiotaRequestAdapterHook

* refactor: Introduced async get_async_conn and keep original synced get_conn method for backward compatibility

* refactor: Introduced async get_async_conn and keep original synced get_conn method for backward compatibility

* refactor: Refactor PowerBI triggers like MSGraph

* refactor: Generate class names dynamically in PowerBI triggers

* refactor: Fixed some mypy issues in PowerBITrigger

* refactor: Fixed test_serialize

* refactor: Reformatted test_get_conn

* refactor: Added docstring to BasePowerBITrigger

* refactor: Replaced DeprecationWarning with AirflowProviderDeprecationWarning

* refactor: Fixed patching of 2 remaining tests

* refactor: Re-added deprecated get_conn methods in triggers

* refactor: Fixed static checks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants