Make Azure dependency optional #2170
Conversation
STARTING TEST [ lint # 2 ]Logs will be available when test completes in the following link: Impatient try: |
STARTING TEST [ pr-build # 1 ]Logs will be available when test completes in the following link: Impatient try: |
SUCCESS [lint # 2] TEST PASSED ✅Well done! 😎 |
SUCCESS [pr-build # 1] TEST PASSED ✅Well done! 😎 |
|
/test integration |
STARTING TEST [ integration # 4 ]Logs will be available when test completes in the following link: Impatient try: |
STARTING TEST [ notebooks # 3 ]Logs will be available when test completes in the following link: Impatient try: |
jenkins-x.yml
Outdated
| - update_package | ||
| - install_dev | ||
| - test | ||
| - TOX_TEST=azure |
| if _GCS_PRESENT: | ||
| from google.auth import exceptions | ||
| from google.cloud import storage | ||
|
|
||
| if _AZURE_PRESENT: | ||
| from azure.storage.blob import BlockBlobService |
There was a problem hiding this comment.
Just wonder, we make these imports dependent on the library presence but we don't check bellow when they are used.
Could it lead to some obscure undefined name errors?
Would it be good idea to check bellow for values of _GCS_PRESENT / _AZURE_PRESENT.
I guess it's because we do not want to modify storage.py as we gonna drop our copy of it and use upstream soon?
There was a problem hiding this comment.
Yeah, that's the main reason. To keep these changes simple.
We do print a log message already if Azure or any of the other optional dependencies are not there, which is handled here:
I also imagine that the logic to check the value of _AZURE_PRESENT inside Storage.py could be a bit more nuanced. For example, what happens if we try to use it but it's not there, should we just raise an exception or fail silently?
There was a problem hiding this comment.
Hmm... If it fail silenty it'd mean it didn't download required files so we will get errors somewhere else that may be harder to debug.
Right now it will raise NameError (I guess) -> the only better suggestion I could have would be to raise some custom DependencyError with more informant error message.
Anyway, that would be only potential improvement and as current logic was already present for _GCS_PRESENT it shouldn't stop this from being merged.
SUCCESS [notebooks # 3] TEST PASSED ✅Well done! 😎 |
|
/test this |
1 similar comment
|
/test this |
STARTING TEST [ pr-build # 5 ]Logs will be available when test completes in the following link: Impatient try: |
STARTING TEST [ pr-build # 6 ]Logs will be available when test completes in the following link: Impatient try: |
|
/test this |
STARTING TEST [ pr-build # 7 ]Logs will be available when test completes in the following link: Impatient try: |
|
/test this |
1 similar comment
|
/test this |
STARTING TEST [ pr-build # 8 ]Logs will be available when test completes in the following link: Impatient try: |
STARTING TEST [ pr-build # 9 ]Logs will be available when test completes in the following link: Impatient try: |
|
/test this |
|
/test this |
STARTING TEST [ pr-build # 11 ]Logs will be available when test completes in the following link: Impatient try: |
STARTING TEST [ pr-build # 10 ]Logs will be available when test completes in the following link: Impatient try: |
|
/retest |
STARTING TEST [ integration # 12 ]Logs will be available when test completes in the following link: Impatient try: |
SUCCESS [integration # 12] TEST PASSED ✅Well done! 😎 |
|
Looks good to go @adriangonz , probably just needs updating the jenkins-x merge conflicts |
|
Seems it may be simple enough to do it on the UI ^ |
|
Just added ^ |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: axsaucedo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Tue Jul 21 14:48:12 UTC 2020 impatient try |
|
Tue Jul 21 14:48:14 UTC 2020 impatient try |
|
Tue Jul 21 15:20:51 UTC 2020 impatient try |
|
Tue Jul 21 15:20:57 UTC 2020 impatient try |
|
Tue Jul 21 17:15:34 UTC 2020 impatient try |
|
Tue Jul 21 17:15:35 UTC 2020 impatient try |
What this PR does / why we need it:
Make dependency on Azure optional. It can still be installed as:
Which issue(s) this PR fixes:
Fixes #2168
Special notes for your reviewer:
Since we leverage KFServing's model initialiser image (which does contain Azure's lib), this shouldn't impact most users.
Does this PR introduce a user-facing change?: