Skip to content
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

Load botocore i/o methods in executor #1196

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Load botocore i/o methods in executor #1196

wants to merge 1 commit into from

Conversation

chemelli74
Copy link

Description of Change

Follow-up of #1120

Added the option to load in the executor botocore methods that do I/O.
To enable the new approach is enough to call

AioSession(load_executor=True)

nothing changes for all current downstream implementations of this library as default is False and thus is not needed to be specified.

Assumptions

Replace this text with any assumptions made (if any)

Checklist for All Submissions

  • I have added change info to CHANGES.rst
  • If this is resolving an issue (needed so future developers can determine if change is still necessary and under what conditions) (can be provided via link to issue with these details):
    • Detailed description of issue
    • Alternative methods considered (if any)
    • How issue is being resolved
    • How issue can be reproduced
  • If this is providing a new feature (can be provided via link to issue with these details):
    • Detailed description of new feature
    • Why needed
    • Alternatives methods considered (if any)

Checklist when updating botocore and/or aiohttp versions

  • I have read and followed CONTRIBUTING.rst
  • I have updated test_patches.py where/if appropriate (also check if no changes necessary)
  • I have ensured that the awscli/boto3 versions match the updated botocore version

bdraco
bdraco reviewed Sep 3, 2024
View reviewed changes
aiobotocore/client.py
Comment on lines 52 to 55
logger.debug(
"AioClientCreator - Method load_service_model[botocore] could generate I/O. Running in executor: %s",
load_executor,
)
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to add this to the documentation instead

bdraco
bdraco reviewed Sep 3, 2024
View reviewed changes
aiobotocore/session.py
Comment on lines 114 to 117
logger.debug(
"AioSession - Method load_service_model[botocore] could generate I/O. Running in executor: %s",
self.load_executor,
)
Copy link
Member

Choose a reason for hiding this comment

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

As above

jakob-keller
jakob-keller reviewed Sep 3, 2024
View reviewed changes
Copy link
Collaborator

@jakob-keller jakob-keller left a comment

Choose a reason for hiding this comment

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

Thank you for creating the PR.

Having taken a brief look, several concerns prevent me from supporting this proposal at this time:

  1. Unless absolutely necessary, we should avoid making any changes to the signature of upstream functions and classes, in particular public APIs. This proposal introduces several API changes, including to create_client(). While they might well be backwards compatible, they introduce drift and might make it more difficult to stay up-to-date with future botocore releases.
  2. botocore (and by extension aiobotocore) has a powerful configuration mechanism that could (should?) be used instead.
  3. As far as I can tell, the botocore loader is far more complex and has a lot more blocking codepaths than are considered here. Having a fully async loader would be preferable to me. I have started work in that area, but it will be a massive change and I am not sure I can pull through and finish it.

@jakob-keller
Copy link
Collaborator

Here's a recent example of adding a configuration option: #1102

@jakob-keller
Copy link
Collaborator

Check https://github.com/boto/botocore/blob/develop/botocore/loaders.py to get a glimpse of the number of I/O operations that exist and might need to be run in the executor. If you then identify all calling code throughout aiobotocore/botocore the task of avoiding any blocking calls becomes daunting, at least to me.

@jakob-keller jakob-keller added the enhancement New feature or request label Sep 3, 2024
@jakob-keller
Copy link
Collaborator

For your reference, I have pushed a branch with progress on fully async loaders to https://github.com/jakob-keller/aiobotocore/tree/async-loaders. This is nowhere near complete, though.

@chemelli74
Copy link
Author

Here's a recent example of adding a configuration option: #1102

So if I go that way, are you willing to add the change ?

@jakob-keller
Copy link
Collaborator

Here's a recent example of adding a configuration option: #1102

So if I go that way, are you willing to add the change ?

That would only address 1. and 2. I feel we should have a discussion about 3. before more effort is invested in this or any other PR. But that is just my opinion.

@chemelli74
Copy link
Author

Here's a recent example of adding a configuration option: #1102

So if I go that way, are you willing to add the change ?

That would only address 1. and 2. I feel we should have a discussion about 3. before more effort is invested in this or any other PR. But that is just my opinion.

I would say that is a start.
So if we go that way current discovered issues are fixed.
We can always extend or as you are doing rework it later on.

What you think ?

@jakob-keller
Copy link
Collaborator

Here's a recent example of adding a configuration option: #1102

So if I go that way, are you willing to add the change ?

That would only address 1. and 2. I feel we should have a discussion about 3. before more effort is invested in this or any other PR. But that is just my opinion.

I would say that is a start. So if we go that way current discovered issues are fixed. We can always extend or as you are doing rework it later on.

What you think ?

What specific issue are you referencing? When #1120 was discussed, no consensus was reached that this is even an issue with aiobotocore.

@chemelli74
Copy link
Author

Here's a recent example of adding a configuration option: #1102

So if I go that way, are you willing to add the change ?

That would only address 1. and 2. I feel we should have a discussion about 3. before more effort is invested in this or any other PR. But that is just my opinion.

I would say that is a start. So if we go that way current discovered issues are fixed. We can always extend or as you are doing rework it later on.
What you think ?

What specific issue are you referencing? When #1120 was discussed, no consensus was reached that this is even an issue with aiobotocore.

I don't know what to say or add.

The issue is made evident by HA, botocore maintainer wrote that it is a problem with the async code.
The above code fix the issue with the blocking calls inside the event loop

Please help me understanding what is the missing information.

@jakob-keller
Copy link
Collaborator

First of all, I am not aware that a related issue has been raised with aiobotocore. This might well be a technicality, but that would be the ideal place to clarify where the issue lies, if there are any work arounds available and/or whether and how to best address it. Discussing the apparent issue, potential solutions and a proposed fix in the context of a PR has not worked well for #1120 and I feel this PR might be on a similar path. I suggest to open an issue and continue the discussion there. Thank you!

@chemelli74 chemelli74 mentioned this pull request Sep 4, 2024
Open
6 tasks
@chemelli74
Copy link
Author

I suggest to open an issue and continue the discussion there. Thank you!

Done, #1199

@chemelli74 chemelli74 mentioned this pull request Sep 23, 2024
Closed
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakob-keller jakob-keller jakob-keller left review comments

@bdraco bdraco bdraco left review comments

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chemelli74 @jakob-keller @bdraco