Use progress bar in downloading File and FileSet #1620

AleksanderWWW · 2024-01-23T17:52:06Z

Before submitting checklist

Did you update the CHANGELOG? (not for test updates, internal changes/refactors or CI/CD setup)
Did you ask the docs owner to review all the user-facing changes?

codecov · 2024-01-23T17:56:40Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (5ed4e32) 76.58% compared to head (dfe4c65) 74.01%.

Files	Patch %	Lines
...eptune/internal/backends/hosted_file_operations.py	33.33%	8 Missing ⚠️

Additional details and impacted files

@@                   Coverage Diff                   @@
##           dev/fetch-ui-parity    #1620      +/-   ##
=======================================================
- Coverage                76.58%   74.01%   -2.58%     
=======================================================
  Files                      292      292              
  Lines                    14703    14429     -274     
=======================================================
- Hits                     11261    10679     -582     
- Misses                    3442     3750     +308

Flag	Coverage Δ
e2e	`?`
e2e-	`?`
macos	`?`
macos-latest	`?`
py3.10	`?`
py3.7	`74.01% <82.97%> (+0.01%)`	⬆️
py3.7.16	`?`
py3.8	`?`
ubuntu	`73.86% <82.97%> (+0.01%)`	⬆️
ubuntu-latest	`?`
unit	`74.01% <82.97%> (+0.01%)`	⬆️
windows	`73.19% <82.97%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AleksanderWWW · 2024-01-24T15:28:48Z

Additionally:

Added a type alias ProgressBarType to save some typing
Created a function to construct a progress bar given a progress bar type and description string

AleksanderWWW · 2024-01-24T15:37:07Z

E2E ✅

https://github.com/neptune-ai/neptune-client/actions/runs/7641402558

CHANGELOG.md

normandy7 · 2024-01-25T07:45:16Z

src/neptune/handler.py

+            progress_bar: (bool or Type of progress bar, optional): progress bar to be used while downloading assets.
+                If `None` or `True` the default tqdm-based progress bar will be used.
+                If `False` no progress bar will be used.
+                If a type of progress bar is passed, it will be used instead of the default one.


How would you recommend to instruct the end-user on how to accomplish this? Is it correct to say that they can define a class that inherits from ProgressBarCallback and pass its type as the argument? (Asking also for docs purposes)

Yes, just like in the example with click in the docstrings. You can pass this ClickProgressBar.
Maybe link it somehow?

Very good; your example did make it super clear. Just wanted to double-check that I've got it right and that this alias didn't change anything on the user-end.

Co-authored-by: Sabine <sabine.nyholm@neptune.ai>

szysad

Looks good!
All my comments are code quality related so nothing blocking :)

szysad · 2024-02-05T09:25:22Z

src/neptune/typing.py

@@ -80,3 +84,6 @@ def __init__(self, *args: Any, **kwargs: Any) -> None:
    @abc.abstractmethod
    def update(self, *, by: int, total: Optional[int] = None) -> None:
        ...
+
+
+ProgressBarType: TypeAlias = Optional[Union[bool, Type[ProgressBarCallback]]]


I suggest unwrapping ProgressBarType from Optional as its kinda unusual practice. I think that

def f(x: Optional[ProgressBarType] = None) pass

is more clear then

def f(x: ProgressBarType = None) pass

Good point. Done :)

szysad · 2024-02-05T09:28:20Z

src/neptune/handler.py

-    def download(self, destination: str = None) -> None:
+    def download(
+        self,
+        destination: str = None,


destination should have type Optional[str] if you are allowed to pass None.
Not sure why mypy is passing on that

Interesting. I see that it's been here for a long time, so yeah - weird. Worth looking into that. Thanks for noticing! Won't hurt to correct it anyway I guess, so let me do that quickly ;)

szysad · 2024-02-05T09:40:50Z

src/neptune/internal/backends/hosted_file_operations.py

+    with construct_progress_bar(progress_bar, "Fetching fileset...") as bar:
+        with response:
+            with open(target_file, "wb") as f:
+                for chunk in response.iter_content(chunk_size=1024 * 1024):
+                    if chunk:
+                        f.write(chunk)
+                        bar.update(by=len(chunk), total=total_size)


its getting a bit nested. There is cool python trick, you can actually use with with multiple context managers with only one indent :))

total_size = int(response.headers.get("content-length", 0)) with ( construct_progress_bar(progress_bar, "Fetching file...") as bar, response, open(target_file, "wb") as f ): for chunk in response.iter_content(chunk_size=1024 * 1024): if chunk: f.write(chunk) bar.update(by=len(chunk), total=total_size)

its added in python 3.10 so not sure if we can use it tho :(

but there are other ways around it in pre 3.10 versions like

with A() as a, \ B() as b, \ C() as c: doSomething(a,b,c)

more info: https://stackoverflow.com/questions/893333/multiple-variables-in-a-with-statement
python 3.10 doc: https://docs.python.org/3/whatsnew/3.10.html#parenthesized-context-managers

Thought of it too, but unfortunately, it is not compatible with lower python versions, so we cannot support that. I can leave a TODO though to update it once we have 3.10 as the minimum version, what do you think?
Also, a workaround here could be to use try-finally syntax, but I'm not certain if we need that so much

yep I think moving from python 3.7 to newer versions is crucial as we will keep running into issues like that.

1-st seems good, I'm not big fan of try-finally approach (its only nesting at the end).
How about ExitStack have you seen that? Its supported since python 3.3

Didn't know that. Looks promising :)

szysad

LGTM! 🚀

Co-authored-by: Sabine <sabine.nyholm@neptune.ai>

AleksanderWWW added 2 commits January 23, 2024 16:57

Use progress bar in downloading File and FileSet

f535763

fix tests

c5e9929

AleksanderWWW added 2 commits January 24, 2024 14:53

introduce ProgressBarType

474876b

Add progress_bar param to artifact and file series fetching

9701d73

changelog

83941cd

AleksanderWWW requested a review from normandy7 January 24, 2024 15:42

handler download docstring

3ea1650

normandy7 reviewed Jan 25, 2024

View reviewed changes

Update CHANGELOG.md

e610ce6

Co-authored-by: Sabine <sabine.nyholm@neptune.ai>

AleksanderWWW requested a review from szysad January 26, 2024 07:33

szysad approved these changes Feb 5, 2024

View reviewed changes

AleksanderWWW added 2 commits February 5, 2024 13:08

code review

e509ad2

use ExitStack

dfe4c65

AleksanderWWW requested a review from szysad February 5, 2024 14:16

szysad approved these changes Feb 5, 2024

View reviewed changes

AleksanderWWW merged commit 7ef63a2 into dev/fetch-ui-parity Feb 5, 2024
4 checks passed

AleksanderWWW deleted the aw/pb-for-filesets branch February 5, 2024 18:38

Raalsky pushed a commit that referenced this pull request Feb 6, 2024

Use progress bar in downloading File and FileSet (#1620)

f04e526

Co-authored-by: Sabine <sabine.nyholm@neptune.ai>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use progress bar in downloading File and FileSet #1620

Use progress bar in downloading File and FileSet #1620

AleksanderWWW commented Jan 23, 2024 •

edited

codecov bot commented Jan 23, 2024 •

edited

AleksanderWWW commented Jan 24, 2024

AleksanderWWW commented Jan 24, 2024

normandy7 Jan 25, 2024

AleksanderWWW Jan 25, 2024

normandy7 Jan 25, 2024

szysad left a comment

szysad Feb 5, 2024

AleksanderWWW Feb 5, 2024

szysad Feb 5, 2024

AleksanderWWW Feb 5, 2024

szysad Feb 5, 2024

AleksanderWWW Feb 5, 2024

szysad Feb 5, 2024

AleksanderWWW Feb 5, 2024

AleksanderWWW Feb 5, 2024

szysad left a comment

Use progress bar in downloading File and FileSet #1620

Use progress bar in downloading File and FileSet #1620

Conversation

AleksanderWWW commented Jan 23, 2024 • edited

Before submitting checklist

codecov bot commented Jan 23, 2024 • edited

Codecov Report

AleksanderWWW commented Jan 24, 2024

Additionally:

AleksanderWWW commented Jan 24, 2024

E2E ✅

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szysad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szysad left a comment

Choose a reason for hiding this comment

AleksanderWWW commented Jan 23, 2024 •

edited

codecov bot commented Jan 23, 2024 •

edited