Fix coroutine serialization error in PowerBIDatasetRefreshOperator#63829
Conversation
|
cc @morelgeorge |
SameerMesiah97
left a comment
There was a problem hiding this comment.
I think you found the cause but I feel the fix might be at the wrong layer. Also, did you manage to reproduce the bug locally?
Thanks for reviewing! Yes, I reproduced the error locally with the unit test. |
8b7e0c8 to
0593eae
Compare
|
Hello @henry3260, The connection is defined directly in UI. In case the wait_for_completion is set to True, DAG works fine. So, it seems that once you run it synchronously through asyncio.run() then get_async_conn does not work properly. I did a quick test to test this behaviour. I changed line 583 in msgraph hook in method Then wait_for_completion works fine - even for True or False |
0593eae to
395b3a3
Compare
Hi @morelgeorge! Thanks for the information! I think this version is better and will avoid the above error. |
395b3a3 to
9dacf17
Compare
|
Hi @dabla, just checking in to see if we can move forward with this PR. Please let me know if there's anything else you need from my end—I'd be happy to help! |
Looking good, I just would be nice if you could test the double deferral using the execute_operator test helper method, check in test_msgraph of the operators, this helper methods tests the full operator lifcyle with deferrals. That way we are certain the full operator lifecycle works. So an additional test for that would be nice, then I think we are ready to merge. Thx for the effort! |
Add it, thanks dabla! |
b4d7aba to
27e20e5
Compare
|
Hi @dabla, is there anything else I can do to help move this PR forward? |
27e20e5 to
341cd00
Compare
341cd00 to
e499fb1
Compare
|
Hi @dabla Could I get your final review on this when you get a chance? |
vincbeck
left a comment
There was a problem hiding this comment.
I am trying to understand (sorry for the late review). As far as I understand you're waiting regardless of wait_for_completion value right? What's wait_for_completion for now?
first really appreciate your review and your time!
1. |
Oh I understand now, thank you |
|
@dabla Can you review it by any chance? |
|
@dabla Could I get your review when you get a chance? |
|
Overal looking good to me. |
|
@vincbeck I think this one is good to go! |
|
Hi I see that the provider package was updated 2 days ago (13.4.0): Was this fix included in the release? |
Why
When the
PowerBIDatasetRefreshOperatoris executed withwait_for_completion=False, it takes the synchronous"fire-and-forget" path. However, the underlying hook methodtrigger_dataset_refreshis an async function.What
Forced Synchronous Execution: Wrapped the
hook.trigger_dataset_refresh(...)call withasyncio.run(...)inside thewait_for_completion=Falseblock. This ensures the async hook method executes fully within the current synchronous thread and returns the actual string ID.closes: #63811
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.