Skip to content

Implement data api asset download and supporting functions #707

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

Merged
merged 6 commits into from
Oct 12, 2022

Conversation

jreiberkyle
Copy link
Contributor

Implements data asset functions, everything needed to download and validate an asset

Closes #499

@jreiberkyle jreiberkyle self-assigned this Oct 3, 2022
Comment on lines +562 to +596
@respx.mock
@pytest.mark.asyncio
async def test_download_asset_img(tmpdir, open_test_img, session):
dl_url = TEST_DOWNLOAD_URL + '/1?token=IAmAToken'

img_headers = {
'Content-Type': 'image/tiff',
'Content-Length': '527',
'Content-Disposition': 'attachment; filename="img.tif"'
}

async def _stream_img():
data = open_test_img.read()
v = memoryview(data)

chunksize = 100
for i in range(math.ceil(len(v) / (chunksize))):
yield v[i * chunksize:min((i + 1) * chunksize, len(v))]

# populate request parameter to avoid respx cloning, which throws
# an error caused by respx and not this code
# https://github.com/lundberg/respx/issues/130
mock_resp = httpx.Response(HTTPStatus.OK,
stream=_stream_img(),
headers=img_headers,
request='donotcloneme')
respx.get(dl_url).return_value = mock_resp

cl = OrdersClient(session, base_url=TEST_URL)
filename = await cl.download_asset(dl_url, directory=str(tmpdir))

assert Path(filename).name == 'img.tif'
assert os.path.isfile(filename)


Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just a move closer to it's sibling test

@jreiberkyle jreiberkyle marked this pull request as ready for review October 4, 2022 00:05
Copy link
Member

@cholmes cholmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, but it looks like it's not the CLI yet (which is totally fine, python api is the target), which just means I can't really do any testing of this on my own.

print command to report wait status. `download_asset` has reporting built in.

```python
>>> async def download_and_validate():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am against having the added ">>>"s and "..."s in example code because it makes it difficult for users to copy and paste and immediately use. We're also inconsistent with its usage. We add it to many of our examples, but don't have it in the checksum example (line 262 in this MD).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm against it too. It is included here for consistency and we need to do a sweep of the docs to remove these. I'm not sure why it isn't in the checksum example, it probably slipped through a PR. But that isn't a part of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tricky thing is that >>> is kinda normalized in Python and numpy. See https://docs.python.org/3/tutorial/introduction.html#numbers and https://numpydoc.readthedocs.io/en/latest/format.html#examples. So without some doc linting, this usage might continue to creep in. For what it's worth, the Python doc source includes the >>> prompt (see https://github.com/python/cpython/blob/3.10/Doc/tutorial/introduction.rst#numbers) and the website has a js widget to hide it if you want to copy it without.

@jreiberkyle
Copy link
Contributor Author

@kevinlacaille anything else you'd like to bring up in the review or do you feel ok with approving?

Copy link
Contributor

@kevinlacaille kevinlacaille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jreiberkyle jreiberkyle merged commit 3aa0799 into main Oct 12, 2022
@jreiberkyle jreiberkyle deleted the data-asset-499 branch October 12, 2022 17:10
This was referenced Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement data api asset functions
4 participants