Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Add AWS ECR support (round two) #1055
base: main
Are you sure you want to change the base?
Add AWS ECR support (round two) #1055
Changes from 16 commits
7c74f8a
81f92ca
7d6f775
b0b438c
6cb5b6a
884af25
e25e597
56dd9cf
bd961a8
af56e93
296772b
ade3a8b
c1c37dd
b9ecea4
f9fd86c
a5edcd3
a9aa576
468bffc
1047f9e
88e4914
8309e97
c6939fe
019a9fe
00048e3
a74d814
dddecf8
a729986
95a2f96
9a4ad32
dfcb921
a2a670a
25faed2
60c8ed4
6e6a61c
b9877a7
a5f21e8
54c1b64
6266cdd
dc676c4
5a4894a
eeb7661
ad97b5a
e7624d9
378f539
762443e
40ddeeb
42a28bf
7cf26bf
d9a983f
482cdb9
591be68
c2b3f6e
56f9e81
6fbc789
8a25e45
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is boto3 async?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not officially. There are third-party wrappers like aioboto3 - https://github.com/terrycain/aioboto3 - but we may not want to go there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BinderHub is written using tornado so we can't make calls over the network with a library which blocks. If we do then all of the BinderHub process will block while that network request is happening. We either need to use a threadpool to execute the boto3 calls or use a async library that you can
await
.Depending on the complexity of the requests you are making another option would be to implement the HTTP call yourself using the
AsyncHTTPClient
class.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. There is really only one request that needs to be made currently and it's relatively simple to do ourselves. Unless you see a value in integrating something like
aioboto3
we can implement the request ourselves usingAsyncHTTPClient
. Drawing inspiration from elsewhere, it seems this is the route that FargateSpawner for JupyterHub went as well when making AWS calls - ref: https://github.com/uktrade/fargatespawner/blob/c614a54ffd80d0fb8886d1ef9e8de2c938de7759/fargatespawner/fargatespawner.py#L322-L346.If you approve of this path, I will start work on implementing and testing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After biting into AWS's request signing process - ref: https://docs.aws.amazon.com/general/latest/gr/signature-version-4.html - I realized heading down the request reimplementation may not be the best approach. Additionally, the kubernetes call is also not async, so I switched to using a threadpool as you suggested. It has now been implemented and tested.