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
[Storage] Azure blob storage support #3032
[Storage] Azure blob storage support #3032
Changes from 132 commits
d867646
4c49ed4
1f2c584
1da2c76
3ab8095
d7a0a26
0820037
32ef2ad
9126119
52787e8
a4ce470
1dc2be6
dd3646d
0ecd1ad
02e06b5
9765849
8200265
c7ce2d5
a156b56
212e003
1a0ad06
6e76b7f
9975596
6becf33
6350fdd
7a1fe65
23977b9
97fc0eb
c56fa32
d23dbb0
c7ed6fb
a78b0f9
861b718
3cba1b6
7ba8b88
8aaed84
5e85fdc
cb1e6a8
d036cd8
95736e4
69539ed
8411fff
d0d6105
3bcb2cb
b9c21be
f1a072e
bc1534d
524a47f
1c96055
b355d25
9e52672
46111f7
e8bc823
47b6909
f23d806
1570022
2aa1e1f
85e323a
e84c46f
bac7319
6e612c9
b67e87c
c994cfc
2f727ac
4c5938a
0f810e0
881b0d2
34a684c
777cd8a
a607874
eaf3058
7494600
696b4c6
8ec5c08
054efae
f856063
1612af4
70ae2b3
d63fc7a
be1cfaa
5a1dd87
eb933df
8e94496
fa113d0
5df34de
de3d8a8
806320c
63dab29
b9dfa47
59d30fb
119e7e5
ecbab0e
6640c15
fb61be4
144a60d
56f8953
408c240
720fe38
80e1bce
c923575
e86e24d
a1940a2
4e5442f
a877d7c
508b8e1
d9b70d4
3b771dc
113cb12
401796b
fe83261
613618e
f30291f
dacf597
6dd9d31
e5cc383
892b504
aea8316
3e8c96e
d58a10b
078ee52
9512449
01b48a4
2bd5ec7
0b16763
7ec0f68
7b3010c
78a2533
7f6ad76
cd33c18
13f90b4
6a8db72
5c67690
95fa03d
b864f42
08adcde
493e300
75d0cda
89645ed
cb606ef
98fcd5f
1a15411
f494725
6d0e128
f8ecddc
e52f797
a6691ed
41d1000
0cd9d73
33fff63
78c5fd3
db9fa49
3b89bff
9f48823
f233cb2
5fe2d60
b5e2cc5
f87de9d
b4421d0
6b12bff
5909311
4c9ac44
bfee828
691582e
64c770a
908acb4
c320575
fb3d48e
076ef01
3e75bdf
e1c56f0
f711911
d5c4e1b
7fefe7b
b93adb1
3f81d7c
3c05ff7
ccecf24
5b1689f
f40604b
642a258
e212ea2
beba84a
55a5e72
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.
do we need to add the
msgraph
module in the argument of@common.load_lazy_modules
?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.
Hm I'm thinking not.
msgraph
is only used when Azure storage is in use, butget_client
method is not just for storage. Doesn't addingmsgraph
to_LAZY_MODULES
make it to importmsgraph
as soon asget_client
is called regardless of storage usage? This makes me hesitant. What do you think? I may not be the best person to answer this.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.
Good point. Lets wait other reviewer's opinion.
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.
Why do we need to distinguish whether the container URL is public or private? Can't we just always pass
credential
in the argument?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.
As discussed offline, added more comment at f87de9d
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.
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.
The colon should do the work in this case. I feel like colons and quotation marks are redundant in this case.
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.
nit: separate it into two lines?
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.
I left it as a single line for readability since there are multiple commands ran.
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.
this two functions have a lot common codes. could we try to add a wrapper to increase code reuse
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.
I'll leave this at it is to keep the interface consistent with the other storage classes. Also, the interface for uploading codes from local to storage has the two function for file and dir separated as well. I think the consistency makes sense.
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.
I mean this, i.e. still keep 2 interface but wrap the shared code into a helper function and call it with different args.
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.
Thanks for the elaboration! Refactored at eb933df. But I'm not sure if this implementation is the most optimal way as the number of line of codes did not reduce much and perhaps it's more messy. I'd be happy to get more feedbacks on this.
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.
Thanks! Will take a look soon.
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.
@cblmemo If there isn't a way to reduce this code further, this should be converted back to how it was. Readability seems to be worse compared to how it was even though the line of codes did not change. All the other storages seems to not share code for the same reason. What do you think?
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.
I think it still reusing a lot of code, and if we don't need to repeat the
blob_path
twice, the last few lines could also be simplified fromto
converted_source = f'{source}/?{sas_token}'
, assuming we also remove the
?
from container sas token generation.