Load botocore i/o methods in executor

[chemelli74]

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]

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

[thehesiod]

indeed

[bdraco]

As above

[jakob-keller]

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]

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

[jakob-keller]

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]

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]

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]

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]

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]

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]

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]

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]

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

Done, #1199

[thehesiod]

instead of this being a bool, you should just pass in the executor you want to use for the loader, that way you avoid defaulting to creating a ton of threads