refactor(artifacts): consolidate hash utilities into lib.hashutil #4525

moredatarequired · 2022-11-23T08:53:59Z

Description

Consolidate the various helper functions for getting the MD5 hash of strings or files as hex or base64 encoded values; move all of them to util and add unit testing specifically for those functions. De-duplicate re-definitions.

Testing

New unit tests added where coverage was only incidental by way of testing something else. Other than that, a pure refactor, rely on existing tests.

Checklist

Include reference to internal ticket "Fixes WB-NNNN" and/or GitHub issue "Fixes #NNNN" (if applicable)
Ensure PR title compliance with the conventional commits standards

For some reason on my machine the calls to don't work consistently; this is faster and more reliable as a way to ensure files are written with sequential c/m/a times.

wandb/util.py

kptkin · 2022-11-23T16:15:21Z

Nice fix! Thanks!

* Use a fixture instead of recreating the artifact cache * Move s3 handler cache test with the rest of the cache tests * Mark test_write_parallel and cache_cleanup_allows_upload as flaky

#4649) * Subsume ArtifactEntry into ArtifactManifestEntry

commit d244d07 Author: Katia Patkin <87335417+kptkin@users.noreply.github.com> Date: Tue Dec 27 13:31:20 2022 -0800 style(public-api): format public file with proper formating (#4697) fix format commit 3dc8584 Author: Janosh Riebesell <janosh.riebesell@gmail.com> Date: Tue Dec 27 10:21:41 2022 -0800 feat(sdk): add `exist_ok=False` to `file.download()` (#4564) * add exist_ok=False to file.download() to avoid raising ValueError if file exists but you don't want to redownload also add type annotations * handle case file exists and exist_ok==True in File.download() * point out exist_ok=True option in file already exists error * tweak if/else paths Co-authored-by: Katia Patkin <87335417+kptkin@users.noreply.github.com> commit 4a4651e Author: Vish Rajiv <8609620+vwrj@users.noreply.github.com> Date: Fri Dec 23 14:27:16 2022 -0800 fix(artifacts): artifact.version should be the version index from the associated collection (#4486) fix(artifacts): artifact.version should be the version index from the associated collection Co-authored-by: Hugh Wimberly <hugh.wimberly@wandb.com> Co-authored-by: Dmitry Duev <dmitryduev@users.noreply.github.com> commit a7de372 Author: Noah Luna <15202580+ngrayluna@users.noreply.github.com> Date: Fri Dec 23 14:19:14 2022 -0800 docs(sdk): Removed less than, greater than characters from dosctrings… (#4687) docs(sdk): Removed less than, greater than characters from dosctrings. Re: It breaks the new doc engine Docusuarus. commit a45629d Author: Noah Luna <15202580+ngrayluna@users.noreply.github.com> Date: Fri Dec 23 14:18:00 2022 -0800 docs(sdk): Fixed typo in docstring for data_types.Objects3D (#4543) Fixed typo in docstring for data_types.Objects3D commit 6df4b88 Author: Dmitry Duev <dmitryduev@users.noreply.github.com> Date: Fri Dec 23 13:16:16 2022 -0800 test(integrations): fix import tests (#4690) test(integrations): fix import tests commit 8da62bb Author: Hugh Wimberly <hugh.wimberly@wandb.com> Date: Fri Dec 23 11:46:00 2022 -0800 refactor(artifacts): consolidate hash utilities into lib.hashutil (#4525) * Move all MD5 and base64 encode utilities to lib.hashutil * Remove duplicate function definitions * Refactor common code out into small functions * Use hypothesis to for testing * Remove unused code * Update type annotations for hash types commit eb70114 Author: Hugh Wimberly <hugh.wimberly@wandb.com> Date: Thu Dec 22 09:44:23 2022 -0800 test(artifacts): improve storage handler test coverage (#4674) * Add test case for adding a file to a finalized artifact * Add test case for caching local file references * Add GCS and WBArtifact storage handler tests commit 7106d17 Author: Hugh Wimberly <hugh.wimberly@wandb.com> Date: Wed Dec 21 19:36:31 2022 -0800 fix(artifacts): get digest directly instead of from the manifests' manifest (#4681) Reference digest directly instead of though the manifests' manifest commit 0a21fd1 Author: KyleGoyette <kdgoyette@gmail.com> Date: Wed Dec 21 14:37:03 2022 -0800 feat(launch): Default to using model-registry project for agent and launch_add (#4613) commit 84be24c Author: Griffin Tarpenning <griffin.tarpenning@wandb.com> Date: Wed Dec 21 13:20:02 2022 -0800 chore(launch): remove fallback resource when not specified for a queue (#4637) commit cf7df1d Author: speezepearson <speezepearson@users.noreply.github.com> Date: Tue Dec 20 17:10:13 2022 -0800 test(sdk): add tests for Api.upload_file_retry (#4639) commit c1ad0c0 Author: Hugh Wimberly <hugh.wimberly@wandb.com> Date: Tue Dec 20 16:08:37 2022 -0800 refactor(artifacts): use ArtifactEntry directly instead of subclassing (#4649) * Subsume ArtifactEntry into ArtifactManifestEntry commit 491fc59 Author: speezepearson <speezepearson@users.noreply.github.com> Date: Tue Dec 20 15:26:54 2022 -0800 test(sdk): add unit tests for filesync.StepUpload (#4652) * add unit tests for filesync.StepUpload commit 4e667d0 Author: Hugh Wimberly <hugh.wimberly@wandb.com> Date: Tue Dec 20 15:02:09 2022 -0800 fix(artifacts): correctly handle url-encoded local file references. (#4665) * Add test case for path names needing correct urldecoding * Use local_fil_uri_to_path instead of urlparse * Fix existing tests that used relative path uris instead of absolute paths

) * Move all MD5 and base64 encode utilities to lib.hashutil * Remove duplicate function definitions * Refactor common code out into small functions * Use hypothesis to for testing * Remove unused code * Update type annotations for hash types

commit d244d07 Author: Katia Patkin <87335417+kptkin@users.noreply.github.com> Date: Tue Dec 27 13:31:20 2022 -0800 style(public-api): format public file with proper formating (#4697) fix format commit 3dc8584 Author: Janosh Riebesell <janosh.riebesell@gmail.com> Date: Tue Dec 27 10:21:41 2022 -0800 feat(sdk): add `exist_ok=False` to `file.download()` (#4564) * add exist_ok=False to file.download() to avoid raising ValueError if file exists but you don't want to redownload also add type annotations * handle case file exists and exist_ok==True in File.download() * point out exist_ok=True option in file already exists error * tweak if/else paths Co-authored-by: Katia Patkin <87335417+kptkin@users.noreply.github.com> commit 4a4651e Author: Vish Rajiv <8609620+vwrj@users.noreply.github.com> Date: Fri Dec 23 14:27:16 2022 -0800 fix(artifacts): artifact.version should be the version index from the associated collection (#4486) fix(artifacts): artifact.version should be the version index from the associated collection Co-authored-by: Hugh Wimberly <hugh.wimberly@wandb.com> Co-authored-by: Dmitry Duev <dmitryduev@users.noreply.github.com> commit a7de372 Author: Noah Luna <15202580+ngrayluna@users.noreply.github.com> Date: Fri Dec 23 14:19:14 2022 -0800 docs(sdk): Removed less than, greater than characters from dosctrings… (#4687) docs(sdk): Removed less than, greater than characters from dosctrings. Re: It breaks the new doc engine Docusuarus. commit a45629d Author: Noah Luna <15202580+ngrayluna@users.noreply.github.com> Date: Fri Dec 23 14:18:00 2022 -0800 docs(sdk): Fixed typo in docstring for data_types.Objects3D (#4543) Fixed typo in docstring for data_types.Objects3D commit 6df4b88 Author: Dmitry Duev <dmitryduev@users.noreply.github.com> Date: Fri Dec 23 13:16:16 2022 -0800 test(integrations): fix import tests (#4690) test(integrations): fix import tests commit 8da62bb Author: Hugh Wimberly <hugh.wimberly@wandb.com> Date: Fri Dec 23 11:46:00 2022 -0800 refactor(artifacts): consolidate hash utilities into lib.hashutil (#4525) * Move all MD5 and base64 encode utilities to lib.hashutil * Remove duplicate function definitions * Refactor common code out into small functions * Use hypothesis to for testing * Remove unused code * Update type annotations for hash types commit eb70114 Author: Hugh Wimberly <hugh.wimberly@wandb.com> Date: Thu Dec 22 09:44:23 2022 -0800 test(artifacts): improve storage handler test coverage (#4674) * Add test case for adding a file to a finalized artifact * Add test case for caching local file references * Add GCS and WBArtifact storage handler tests commit 7106d17 Author: Hugh Wimberly <hugh.wimberly@wandb.com> Date: Wed Dec 21 19:36:31 2022 -0800 fix(artifacts): get digest directly instead of from the manifests' manifest (#4681) Reference digest directly instead of though the manifests' manifest commit 0a21fd1 Author: KyleGoyette <kdgoyette@gmail.com> Date: Wed Dec 21 14:37:03 2022 -0800 feat(launch): Default to using model-registry project for agent and launch_add (#4613) commit 84be24c Author: Griffin Tarpenning <griffin.tarpenning@wandb.com> Date: Wed Dec 21 13:20:02 2022 -0800 chore(launch): remove fallback resource when not specified for a queue (#4637) commit cf7df1d Author: speezepearson <speezepearson@users.noreply.github.com> Date: Tue Dec 20 17:10:13 2022 -0800 test(sdk): add tests for Api.upload_file_retry (#4639) commit c1ad0c0 Author: Hugh Wimberly <hugh.wimberly@wandb.com> Date: Tue Dec 20 16:08:37 2022 -0800 refactor(artifacts): use ArtifactEntry directly instead of subclassing (#4649) * Subsume ArtifactEntry into ArtifactManifestEntry commit 491fc59 Author: speezepearson <speezepearson@users.noreply.github.com> Date: Tue Dec 20 15:26:54 2022 -0800 test(sdk): add unit tests for filesync.StepUpload (#4652) * add unit tests for filesync.StepUpload commit 4e667d0 Author: Hugh Wimberly <hugh.wimberly@wandb.com> Date: Tue Dec 20 15:02:09 2022 -0800 fix(artifacts): correctly handle url-encoded local file references. (#4665) * Add test case for path names needing correct urldecoding * Use local_fil_uri_to_path instead of urlparse * Fix existing tests that used relative path uris instead of absolute paths

moredatarequired added 14 commits November 22, 2022 21:20

test(artifacts): flush writes instead of sleeping

cb2d9aa

For some reason on my machine the calls to don't work consistently; this is faster and more reliable as a way to ensure files are written with sequential c/m/a times.

Remove unused 'md5_string'

3c8042e

Move md5_string to util

974a2cb

Remove extra defintion of md5_string

d9602ea

Move b64_string_to_hex to util

4f6f584

Use hypothesis to test b64_string_to_hex

df1a2c2

Refactor common code out into small function

dc2d27a

Move md5_hash_file to util

93e9a95

Combine md5_file and md5_file_hash

7ffa2e5

Use multiple arguments instead of a list

060d0bf

Use 'sorted' instead of creating new list

2fddb72

Move md5_files_b64 to util

d2453c3

Remove unused code

d441bf5

Import reordering

a28b1a6

moredatarequired requested a review from a team as a code owner November 23, 2022 08:53

github-actions bot added the cc-refactor label Nov 23, 2022

moredatarequired added 6 commits November 23, 2022 00:54

Remove unused imports

de931fd

Correct definition of RawMD5

5faab1b

Inspect type(md5) to deal with different python versions

0e6064d

Format with black

1cdbaff

Mypy reported type for hashlib.md5

43587aa

Mypy fix for Python <3.7

2d84fc3

kptkin reviewed Nov 23, 2022

View reviewed changes

wandb/util.py Outdated Show resolved Hide resolved

moredatarequired added 6 commits November 23, 2022 10:06

Merge branch 'main' into artifacts/refactor-consolidate-util-funcs

7256d6d

Move hash utilities to hashutil.py

ccf993e

Remove extra defintion of B64MD5/ETag/HexMD5

7e9547a

Sort imports

b7b7dae

Merge branch 'main' into artifacts/refactor-consolidate-util-funcs

dac1365

Sort imports

3691219

moredatarequired and others added 6 commits December 20, 2022 14:35

refactor(artifacts): use a pytest fixture for the artifact cache (#4648)

76d17e4

* Use a fixture instead of recreating the artifact cache * Move s3 handler cache test with the rest of the cache tests * Mark test_write_parallel and cache_cleanup_allows_upload as flaky

Merge branch 'main' into artifacts/refactor-consolidate-util-funcs

b3f987d

Fix merge conflict

cc6b753

Merge branch 'main' into artifacts/refactor-consolidate-util-funcs

34878ec

Resort imports

d493e9f

Merge branch 'main' into artifacts/refactor-consolidate-util-funcs

d515238

moredatarequired disabled auto-merge December 20, 2022 23:55

moredatarequired removed the request for review from kptkin December 20, 2022 23:55

moredatarequired and others added 9 commits December 20, 2022 16:29

Fix merge conflicts

992fb78

Add __post_init__ back in that was removed in a merge conflict

1113139

refactor(artifacts): use ArtifactEntry directly instead of subclassing (

c40cab6

#4649) * Subsume ArtifactEntry into ArtifactManifestEntry

test(sdk): add tests for Api.upload_file_retry (#4639)

8aeb557

Fix *another* merge conflict

5585c85

Merge with main

d5c7b46

Merge branch 'main' into artifacts/refactor-consolidate-util-funcs

3731639

Merge branch 'main' into artifacts/refactor-consolidate-util-funcs

e80b159

Merge branch 'main' into artifacts/refactor-consolidate-util-funcs

ca7ec3d

github-actions bot added cc-refactor and removed cc-refactor labels Dec 22, 2022

kptkin approved these changes Dec 23, 2022

View reviewed changes

moredatarequired merged commit 8da62bb into main Dec 23, 2022

moredatarequired deleted the artifacts/refactor-consolidate-util-funcs branch December 23, 2022 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(artifacts): consolidate hash utilities into lib.hashutil #4525

refactor(artifacts): consolidate hash utilities into lib.hashutil #4525

moredatarequired commented Nov 23, 2022 •

edited

kptkin commented Nov 23, 2022

refactor(artifacts): consolidate hash utilities into lib.hashutil #4525

refactor(artifacts): consolidate hash utilities into lib.hashutil #4525

Conversation

moredatarequired commented Nov 23, 2022 • edited

Description

Testing

Checklist

kptkin commented Nov 23, 2022

moredatarequired commented Nov 23, 2022 •

edited