Skip to content

Add setter for blob_service_client in WasbHook to support async injection #54219

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
jason810496 merged 10 commits into from
Aug 19, 2025
Merged

Add setter for blob_service_client in WasbHook to support async injection #54219

jason810496 merged 10 commits into from
Aug 19, 2025
+70 −25

Conversation

@fweilun
Copy link
Contributor

@fweilun fweilun commented Aug 7, 2025

Resolve: #53647 (comment)

Originally, self.blob_service_client was set to None and later assigned via the get_async_conn() function, which does not follow the @cached_property pattern.
Therefore, we added a setter to allow direct injection into WasbAsyncHook.

prdai
prdai approved these changes Aug 8, 2025
View reviewed changes
Copy link
Contributor

@prdai prdai left a comment

Choose a reason for hiding this comment

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

other than the unrelated file, everything else LGTM!

@fweilun fweilun force-pushed the rework-microsoft-azure-blob-service-client branch from b3ee191 to 72d6e4c Compare August 11, 2025 09:05
@fweilun fweilun force-pushed the rework-microsoft-azure-blob-service-client branch from 72d6e4c to 8f9b60d Compare August 11, 2025 09:33
Lee-W
Lee-W reviewed Aug 11, 2025
View reviewed changes
Copy link
Member

@Lee-W Lee-W left a comment •
edited
Loading

Choose a reason for hiding this comment

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

will need some unit tests

jason810496
jason810496 reviewed Aug 12, 2025
View reviewed changes
Copy link
Member

@jason810496 jason810496 left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for the PR, but the unit test still needs fix:

Error logs:
https://github.com/apache/airflow/actions/runs/16898665165/job/47873847733?pr=54219

@fweilun
Copy link
Contributor Author

fweilun commented Aug 12, 2025

Nice! Thanks for the PR, but the unit test still needs fix:

Error logs: https://github.com/apache/airflow/actions/runs/16898665165/job/47873847733?pr=54219

Resolved. Thanks for your review. Should I also work on silencing mypy warnings?

@fweilun fweilun closed this Aug 12, 2025
@fweilun fweilun reopened this Aug 12, 2025
@fweilun fweilun force-pushed the rework-microsoft-azure-blob-service-client branch from 959f51b to fc5cb2e Compare August 12, 2025 07:37
jason810496
jason810496 reviewed Aug 13, 2025
View reviewed changes
Copy link
Member

@jason810496 jason810496 left a comment

Choose a reason for hiding this comment

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

Should I also work on silencing mypy warnings?

It would be great to resolve the mypy errors if possible, if not then maybe we have to silence mypy warnings.

It seems that some of the mypy errors raise in CI should be resolved by adding AsyncContainerClient in the type annotation.

For example:
https://github.com/apache/airflow/pull/54219/files#diff-9774207ad78091f5ced7f3fa5e1e8f2b3d19abb50de753120b70efe25efd0036R230-R238

adding AsyncContainerClient in the type annotation should resolve the mypy error at line 238.

@jason810496
Copy link
Member

jason810496 commented Aug 13, 2025

Or maybe we can split the attributes by decoupling the cache attributes for normal client and async client:

  • self._blob_service_client: BlobServiceClient
  • self._async_blob_service_client: AsyncBlobServiceClient

IMO, this might be more simpler to avoid mypy warning.

@fweilun fweilun force-pushed the rework-microsoft-azure-blob-service-client branch from fc5cb2e to a898c3c Compare August 13, 2025 14:35
@fweilun
Copy link
Contributor Author

fweilun commented Aug 14, 2025 •
edited
Loading

HI @jason810496 , I’ve resolved the errors. The TODO message suggests using self.blob_service_client for both WasbAsyncHook and WasbHook, so I followed the same approach. Thanks a lot for your review and guidance.

Lee-W
Lee-W approved these changes Aug 15, 2025
View reviewed changes
Copy link
Member

@Lee-W Lee-W left a comment •
edited
Loading

Choose a reason for hiding this comment

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

the improvement I suggest is more like a nitpick, non-blocker

jason810496
jason810496 reviewed Aug 17, 2025
View reviewed changes
Copy link
Member

@jason810496 jason810496 left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

I just checked locally, adding BlobServiceClient to the TypeVar and explicitly casting the container variable to the corresponding type resolves the Mypy error.

@fweilun fweilun force-pushed the rework-microsoft-azure-blob-service-client branch from 280ae99 to b63957a Compare August 19, 2025 02:03
@fweilun
Copy link
Contributor Author

fweilun commented Aug 19, 2025

HI @jason810496 , I applied all the changes. Thanks for your help!

@jason810496 jason810496 merged commit 6e92943 into apache:main Aug 19, 2025
75 checks passed
mangal-vairalkar pushed a commit to mangal-vairalkar/airflow that referenced this pull request Aug 30, 2025
@fweilun @mangal-vairalkar
Add setter for blob_service_client in WasbHook to support async injec…
73b8a41 
…tion (apache#54219)

* Convert cached_property to manual caching and add blob_service_client setter for WasbAsyncHook

* Add testcase and error handle

* Adding type implication

* Add _blob_service_client init for WasbAsyncHook

* Silence mypy warnings

* Silence mypy

* Add container type for pytest

* Add type check function

* Silence mypy

* Generalize check_for_variable_type function
@eladkal eladkal mentioned this pull request Sep 5, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@josh-fell josh-fell josh-fell left review comments

@jason810496 jason810496 jason810496 approved these changes

@Lee-W Lee-W Lee-W approved these changes

@ashb ashb Awaiting requested review from ashb

@kaxil kaxil Awaiting requested review from kaxil

@amoghrajesh amoghrajesh Awaiting requested review from amoghrajesh

@ephraimbuddy ephraimbuddy Awaiting requested review from ephraimbuddy

@bolkedebruin bolkedebruin Awaiting requested review from bolkedebruin

@eladkal eladkal Awaiting requested review from eladkal

@pierrejeambrun pierrejeambrun Awaiting requested review from pierrejeambrun

@potiuk potiuk Awaiting requested review from potiuk

@gopidesupavan gopidesupavan Awaiting requested review from gopidesupavan

@dstandish dstandish Awaiting requested review from dstandish

@jedcunningham jedcunningham Awaiting requested review from jedcunningham

@Fokko Fokko Awaiting requested review from Fokko

@mik-laj mik-laj Awaiting requested review from mik-laj

@hussein-awala hussein-awala Awaiting requested review from hussein-awala

@mobuchowski mobuchowski Awaiting requested review from mobuchowski

@vincbeck vincbeck Awaiting requested review from vincbeck

@bugraoz93 bugraoz93 Awaiting requested review from bugraoz93

@jscheffl jscheffl Awaiting requested review from jscheffl

@o-nikolas o-nikolas Awaiting requested review from o-nikolas

@uranusjr uranusjr Awaiting requested review from uranusjr

@shahar1 shahar1 Awaiting requested review from shahar1

@romsharon98 romsharon98 Awaiting requested review from romsharon98

@bbovenzi bbovenzi Awaiting requested review from bbovenzi

@ryanahamilton ryanahamilton Awaiting requested review from ryanahamilton

@shubhamraj-git shubhamraj-git Awaiting requested review from shubhamraj-git

@rawwar rawwar Awaiting requested review from rawwar

@XD-DENG XD-DENG Awaiting requested review from XD-DENG

+1 more reviewer

@prdai prdai prdai approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area:providers provider:microsoft-azure Azure-related issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@fweilun @jason810496 @Lee-W @josh-fell @prdai