Skip to content
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

NPZ replacement format (only) #1047

Merged
merged 97 commits into from Jul 14, 2021
Merged

NPZ replacement format (only) #1047

merged 97 commits into from Jul 14, 2021

Conversation

farizrahman4u
Copy link
Contributor

No description provided.

Copy link
Contributor

@verbose-void verbose-void left a comment

Choose a reason for hiding this comment

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

commit docstring suggestions and it looks good

hub/core/serialize.py Outdated Show resolved Hide resolved
hub/core/serialize.py Outdated Show resolved Hide resolved
hub/core/serialize.py Outdated Show resolved Hide resolved
hub/core/serialize.py Outdated Show resolved Hide resolved
farizrahman4u and others added 4 commits July 14, 2021 12:37
Co-authored-by: dyllan <mccreary@dyllan.ai>
Co-authored-by: dyllan <mccreary@dyllan.ai>
Co-authored-by: dyllan <mccreary@dyllan.ai>
Co-authored-by: dyllan <mccreary@dyllan.ai>
Copy link
Contributor

@AbhinavTuli AbhinavTuli left a comment

Choose a reason for hiding this comment

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

Looks good, approving as we need this quickly.
If there's time, assertions should be replaced with exceptions everywhere except for tests.

@farizrahman4u farizrahman4u marked this pull request as ready for review July 14, 2021 09:01
@codecov
Copy link

codecov bot commented Jul 14, 2021

Codecov Report

Merging #1047 (4d03080) into main (b180668) will not change coverage.
The diff coverage is n/a.

❗ Current head 4d03080 differs from pull request most recent head 7007c59. Consider uploading reports for the commit 7007c59 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1047   +/-   ##
=======================================
  Coverage   89.16%   89.16%           
=======================================
  Files          88       88           
  Lines        3998     3998           
=======================================
  Hits         3565     3565           
  Misses        433      433           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b180668...7007c59. Read the comment docs.

@verbose-void
Copy link
Contributor

mac failing, looks like an install error so you can probably just rerun it

@verbose-void
Copy link
Contributor

verbose-void commented Jul 14, 2021

benchmarks

note: these times are for the full script run. for pytorch this includes process spin-up times. was run on my local macbook pro.

mnist upload times (local to local)

this branch (commit d9a846b): 43s
main branch (commit 4d03080): 45s

mnist pytorch times (local, 1 worker)

this branch (commit d9a846b): 17s
main branch (commit 4d03080): 18s

Copy link
Contributor

@verbose-void verbose-void left a comment

Choose a reason for hiding this comment

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

let's make sure this change is 100% necessary before merging

Copy link
Contributor

@verbose-void verbose-void left a comment

Choose a reason for hiding this comment

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

after profiling, we deduced that these serializers are 3x faster for 512x512 data and 3.8x faster for 1024x1024 data in comparison to npz. let's get it merged!

@farizrahman4u farizrahman4u merged commit 07fb8d2 into main Jul 14, 2021
@farizrahman4u farizrahman4u deleted the fr_serialization branch July 14, 2021 10:37
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.

None yet

3 participants