IA-2932 Add upload to azure functionality by Qi77Qi · Pull Request #362 · DataBiosphere/terra-notebook-utils

Qi77Qi · 2021-09-28T19:39:11Z

https://broadworkbench.atlassian.net/browse/IA-2932

Tested on a terra VM using b.adm.firec account

>>> drs.copy("drs://jade.datarepo-dev.broadinstitute.org/v1_0c86170e-312d-4b39-a0a4-2a2bfaa24c7a_c0e40912-8b14-43f6-9a2f-b278144d0060", "https://qijlbdgpc4zqdee.blob.core.windows.net/qi-test-container/qi-blob-10-6")
2021-10-06 06:36:51::INFO  Using Access Key
2021-10-06 06:36:51::INFO  Request URL: 'https://qijlbdgpc4zqdee.blob.core.windows.net/qi-test-container/qi-blob-10-6'/nRequest method: 'PUT'/nRequest headers:/n    'x-ms-blob-type': 'REDACTED'/n    'Content-Length': '62043448'/n    'If-None-Match': '*'/n    'x-ms-version': 'REDACTED'/n    'Content-Type': 'application/octet-stream'/n    'Accept': 'application/xml'/n   'User-Agent': 'azsdk-python-storage-blob/12.8.1 Python/3.7.10 (Linux-5.4.144+-x86_64-with-debian-buster-sid)'/n    'x-ms-date': 'REDACTED'/n    'x-ms-client-request-id': '5f7692cc-26d4-11ec-9ae3-0242ac120005'/n    'Authorization': 'REDACTED'/nA body is sent with the request
2021-10-06 06:36:53::INFO  Response status: 201/nResponse headers:/n    'Content-Length': '0'/n    'Content-MD5': 'REDACTED'/n    'Last-Modified': 'Wed, 06 Oct 2021 18:36:53 GMT'/n    'ETag': '"0x8D988F844B358B1"'/n    'Server': 'Windows-Azure-Blob/1.0 Microsoft-HTTPAPI/2.0'/n    'x-ms-request-id': 'cff454e3-401e-00ef-77e1-ba31ac000000'/n    'x-ms-client-request-id': '5f7692cc-26d4-11ec-9ae3-0242ac120005'/n    'x-ms-version': 'REDACTED'/n    'x-ms-content-crc64': 'REDACTED'/n    'x-ms-request-server-encrypted': 'REDACTED'/n    'Date': 'Wed, 06 Oct 2021 18:36:53 GMT'
 https://qijlbdgpc4zqdee.blob.core.window   100%   [========================================]   59.2MiB   25.6MiB/s   2.31s

cleanup try enable logging err test test test comments add tests merge error

This reverts commit ea2bc77.

DailyDreaming

Hmmm... I'm starting to think that the Google and Azure portions should be a little more separated.

Ideally, you should be able to operate on Google files if you have valid Google credentials and invalid Azure credentials. And vice versa. The same goes for running tests. Azure tests should print a "no azure credentials found message" and skip azure tests. The same goes for Google imo (I can add the Google side).

What do you think about an "azure" TNU_TESTMODE?

It looks like there may need to be some apt dependencies that should at least be added to the README.MD? (sudo apt install python3-gi python3-gi-cairo gir1.2-secret-1:

test_exists (__main__.TestBlobStore) ... EnvironmentCredential.get_token failed: EnvironmentCredential authentication unavailable. Environment variables are not fully configured.
ImdsCredential.get_token failed: ManagedIdentityCredential authentication unavailable, no managed identity endpoint found.
ManagedIdentityCredential.get_token failed: ManagedIdentityCredential authentication unavailable, no managed identity endpoint found.
Runtime dependency of PyGObject is missing.
Depends on your Linux distro, you could install it system-wide by something like:
    sudo apt install python3-gi python3-gi-cairo gir1.2-secret-1
If necessary, please refer to PyGObject's doc:
https://pygobject.readthedocs.io/en/latest/getting_started.html
Traceback (most recent call last):
  File "/home/quokka/venv/lib/python3.9/site-packages/msal_extensions/libsecret.py", line 21, in <module>
    import gi  # https://github.com/AzureAD/microsoft-authentication-extensions-for-python/wiki/Encryption-on-Linux
ModuleNotFoundError: No module named 'gi'

Also, I haven't checked all of the tests, but should the azure blobstore be included in the test_open() test?:

terra-notebook-utils/tests/test_blobstore.py

Line 141 in 843a26b

for bs in (local_blobstore, gs_blobstore, url_blobstore):

terra_notebook_utils/drs.py

DailyDreaming · 2021-10-11T19:21:00Z

terra_notebook_utils/drs.py

        bucket_name, key = _resolve_bucket_target(dst, src_info)
        dst_blob = GSBlob(bucket_name, key)
+    # Azure url looks like https://qijlbdgpc4zqdee.blob.core.windows.net/qi-test-container/subdir/another/qi-blob3
+    if "windows.net" in dst:


I feel like, ideally, we'd have a wasb:// scheme to distiguish these, especially if we want plain https:// support in the future.

Would you consider making this stricter? If so, would something like:

if url.startswth('https://') and url[len('https://'):].split('/')[0].endswith('.blob.core.windows.net'): ...

or even a function like:

def is_azure_url(self, azure_url: str) -> bool: try: self._resolve_azure_blob_path(azure_url) return True except: return False if is_azure_url(uri): ...

Work?

think I'll use your first suggestion..._resolve_azure_blob_path is already being called within the branch, so I think we don't want to call it twice

DailyDreaming · 2021-10-11T19:22:33Z

tests/manual_azure_test.py

+    client.upload_blob(data)
+
+# with open("test", "wb") as data:
+#     data.write(client.download_blob().readall())


the commented-out code is reading data which can still be interesting to test...it's commented out just becuz I was testing just write

Would it make sense to make this its own test?

there is a test for the actual feature in ci, which uses an Azure access key....this manual test is really just for testing out auth/permission with DefaultAzureCredential...we still need to figure out how exactly we do auth still in terra (there's some on-going discussion)

DailyDreaming · 2021-10-11T19:22:45Z

tests/manual_azure_test.py

+    return service
+
+client = get_service("qijlbdgpc4zqdee").get_blob_client("qi-test-container", "qi-blob-1")
+# client = get_service("qinonmanagedapp").get_blob_client("qi-test-container", "qi-blob-1")


I was testing different storage accounts... I'll remove this for now

DailyDreaming · 2021-10-11T19:24:30Z

tests/test_blobstore.py

-                bs.blob(key).put(expected_data)
-                self.assertEqual(expected_data, bs.blob(key).get())
-                bs.blob(key).delete()
+                client = bs.blob(key)


This seems like a blob instead of a client?

it's the client object for doing stuff with the blob

DailyDreaming · 2021-10-11T19:43:33Z

terra_notebook_utils/blobstore/copy_client.py

            _copy_intra_cloud(src_blob, dst_blob, indicator_type)
-        elif isinstance(dst_blob, CloudBlob):
+        elif isinstance(dst_blob, AzureBlob):
+            assert isinstance(src_blob, (URLBlob, GSBlob))


What if the src_blob is a LocalBlob?

I don't know this very well...but it's copied from https://github.com/DataBiosphere/terra-notebook-utils/blob/master/terra_notebook_utils/blobstore/copy_client.py#L73

Hmmm... I'll investigate why this is disallowed then. My preference is to replace plain asserts with exceptions and helpful debug messages, but I don't know why this wouldn't be allowed right now.

Qi77Qi · 2021-10-12T15:24:58Z

It looks like there may need to be some apt dependencies that should at least be added to the README.MD? (sudo apt install python3-gi python3-gi-cairo gir1.2-secret-1

I didn't test things on local laptop..I did this, which relies on a terra image, and that should have all the system dependencies

Qi77Qi · 2021-10-12T16:52:46Z

Also, I haven't checked all of the tests, but should the azure blobstore be included in the test_open() test?:

I'll double check that there's a test for reading data...I don't know if test_open() applies directly

DailyDreaming · 2021-10-12T16:59:22Z

@Qi77Qi The instructions you added were very helpful! Even though most use-cases will be inside of Terra, I'd like to keep instructions for TNU's use outside of Terra, as I think some of the functions, particularly DRS resolution, are quite useful as a stand-alone python API. Right now I think just noting the additional apt dependencies is sufficient and helpful when not in a Terra-like environment.

Qi77Qi · 2021-10-12T18:09:35Z

test_put_get_delete is already testing read/write/delete...not sure exactly what test_open() is validating 🤔

Qi77Qi mentioned this pull request Sep 28, 2021

WIP Add upload to azure functionality #356

Closed

Qi77Qi force-pushed the add-azure branch from c821d63 to 0b6391c Compare September 28, 2021 20:58

Qi77Qi changed the title ~~WIP Add upload to azure functionality~~ IA-2932 WIP Add upload to azure functionality Sep 28, 2021

Qi77Qi force-pushed the add-azure branch 6 times, most recently from 4fe81ac to ea2bc77 Compare October 6, 2021 16:20

Qi77Qi changed the title ~~IA-2932 WIP Add upload to azure functionality~~ IA-2932 Add upload to azure functionality Oct 6, 2021

Qi77Qi requested a review from DailyDreaming October 7, 2021 16:15

Qi77Qi added 12 commits October 8, 2021 14:27

tmp

4f351ea

cleanup try enable logging err test test test comments add tests merge error

test

320155d

fix test

b13f29a

fixes

5fefbd0

don't upgrade to 12.9.0

8710201

styling fixes

b26a9e0

fix syntax

6d7e37f

revert http -> http_session refactor

cbddbbd

attempt to fix

81af085

cleanup

54f6f77

cleanup

a52e835

Revert "attempt to fix"

f1fbf06

This reverts commit ea2bc77.

Qi77Qi force-pushed the add-azure branch from 40075e0 to f1fbf06 Compare October 8, 2021 18:28

bad merge

843a26b

DailyDreaming reviewed Oct 11, 2021

View reviewed changes

DailyDreaming requested a review from mikebaumann October 11, 2021 19:54

address comments

d8154a8

Conversation

Qi77Qi commented Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DailyDreaming left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Qi77Qi commented Oct 12, 2021

Uh oh!

Qi77Qi commented Oct 12, 2021

Uh oh!

DailyDreaming commented Oct 12, 2021

Uh oh!

Qi77Qi commented Oct 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Qi77Qi commented Sep 28, 2021 •

edited

Loading