feat(downloads): allow streaming downloads access to response iterator #1956

TCatshoek · 2022-03-30T21:19:59Z

Allow access to the underlying response iterator when downloading in
streaming mode by specifying action="iterator"

Also update type annotations to support this change

codecov-commenter · 2022-03-31T06:29:01Z

Codecov Report

Merging #1956 (9f01ee5) into main (f9b7c7b) will increase coverage by 0.00%.
The diff coverage is 92.59%.

@@           Coverage Diff           @@
##             main    #1956   +/-   ##
=======================================
  Coverage   94.72%   94.73%           
=======================================
  Files          78       78           
  Lines        5083     5087    +4     
=======================================
+ Hits         4815     4819    +4     
  Misses        268      268

Flag	Coverage Δ
cli_func_v4	`82.42% <66.66%> (-0.01%)`	⬇️
py_func_v4	`81.14% <66.66%> (-0.03%)`	⬇️
unit	`85.80% <51.85%> (-0.03%)`	⬇️

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

Impacted Files	Coverage Δ
gitlab/v4/cli.py	`90.61% <ø> (ø)`
gitlab/v4/objects/jobs.py	`77.27% <50.00%> (ø)`
gitlab/mixins.py	`92.30% <100.00%> (ø)`
gitlab/utils.py	`100.00% <100.00%> (ø)`
gitlab/v4/objects/artifacts.py	`100.00% <100.00%> (ø)`
gitlab/v4/objects/files.py	`100.00% <100.00%> (ø)`
gitlab/v4/objects/packages.py	`96.29% <100.00%> (ø)`
gitlab/v4/objects/projects.py	`90.00% <100.00%> (+0.03%)`	⬆️
gitlab/v4/objects/repositories.py	`83.07% <100.00%> (ø)`
gitlab/v4/objects/snippets.py	`98.00% <100.00%> (ø)`

nejch · 2022-03-31T06:44:19Z

Thank you for all the work here @TCatshoek!

For this to work with typing on 3.7, we'll need to add a conditional dependency on https://pypi.org/project/typing-extensions/.

It would also be great to document this a little and if you could maybe add a specific use case there (e.g. from your issue) a bit to give context :) I think doing this with action="iterator" is fine but I can also poke around to see how other people might do this.

As a bonus, if you have the energy, it'd be great if we could maybe also do a functional test for this. I know we don't have any targeted tests for this but perhaps down here:

python-gitlab/tests/functional/api/test_packages.py

Lines 38 to 62 in 5a1678f

    
           def test_download_generic_package(project): 
        
               package = project.generic_packages.download( 
        
                   package_name=package_name, 
        
                   package_version=package_version, 
        
                   file_name=file_name, 
        
               ) 
        
               assert isinstance(package, bytes) 
        
               assert package.decode("utf-8") == file_content 
        
           def test_download_generic_package_to_file(tmp_path, project): 
        
               path = tmp_path / file_name 
        
               with open(path, "wb") as f: 
        
                   project.generic_packages.download( 
        
                       package_name=package_name, 
        
                       package_version=package_version, 
        
                       file_name=file_name, 
        
                       streamed=True, 
        
                       action=f.write, 
        
                   ) 
        
               with open(path, "r") as f: 
        
                   assert f.read() == file_content

TCatshoek · 2022-03-31T10:34:39Z

@nejch Awesome! I'll work on tests and docs as soon as I have some time.

In the meantime I managed to find a suitable, albeit kind of ugly workaround to get the behavior I want. I'll describe it in the issue I opened at #1955

Allow access to the underlying response iterator when downloading in streaming mode by specifying action="iterator" Update type annotations to support this change

This supports commit c76b3b1 and makes sure it works on python 3.7 as well.

This is used to support Literal from typing on python < 3.8

TCatshoek · 2022-04-09T13:56:04Z

I added the conditional dependency on typing_extensions for python 3.7. Had to shuffle some imports around to make the linter happy, I hope it is ok like this!

Document the usage of the action="iterator" option when downloading artifacts

Tests for the functionality implemented in 4f9807f

TCatshoek · 2022-04-10T09:00:47Z

Added tests and documentation too!

nejch

Just a few suggestions for the docs @TCatshoek and I'll just check if it makes more sense to add another argument, I'll get back to you ASAP. Thanks :)

docs/gl_objects/pipelines_and_jobs.rst

nejch · 2022-04-13T06:20:35Z

requirements.txt

@@ -1,2 +1,3 @@
 requests==2.27.1
 requests-toolbelt==0.9.1
+typing_extensions; python_version<"3.8"


This works in testing, but I assume we'd also need this in setup.py for the installed package for users.

However, let me just take another look because if we're only doing this for Literal, it might make more sense to add a separate argument in addition to action, to keep typing clearer. (Also we'd need to change the docstrings otherwise)

Ah yes, you're right. Let me know what you decide and I will update things accordingly!

@nejch Any news on this?

Thanks for your patience @TCatshoek!

I just took another look at this and I feel we should maybe leave action a Callable only because we do this later:

python-gitlab/gitlab/utils.py

Lines 44 to 46 in 792cee9

for chunk in response.iter_content(chunk_size=chunk_size):

if chunk:

action(chunk)

So I think that might be a bit confusing. How about an explicit iterator: bool = False argument that would follow the streamed logic a bit more? This would also get rid of the typing dependency we discussed as I see some people don't like having even the tiniest extra dependencies.. :)

Thanks again for working on this! Let me know what you think.

Just had some discussion with @JohnVillalovos and we're thinking of intruducing an iterator=True for our list() calls where we currently use as_list=False (which is kind of unclear in what it really does currently), so this would then make it a consistent interfaces as well. So I think a bool with iterator is the way to go,

Sounds good! I'll get to it asap, work has been keeping me busy :)

Cool! No rush. FYI, the list iterator argument has been merged:
#2032

gitlab/v4/objects/snippets.py

Co-authored-by: Nejc Habjan <hab.nejc@gmail.com>

TCatshoek · 2022-04-14T13:16:27Z

Sorry, had to force push because the commit messages from accepting the suggested changes through the web interface were not accepted.

nejch · 2022-06-25T15:28:17Z

Would you still be interested in adapting this for iterator=True @TCatshoek? If so we could merge it before the 28th when the next release is scheduled. :)

TCatshoek · 2022-06-25T16:08:06Z

Yeah! I think I should have some time this weekend, I'll try to get it done before the 28th

This reverts commit 8cc4cfc.

…thon 3.7" This reverts commit efd8b48.

… iterator" This reverts commit 4f9807f.

…r downloads Instead of having action="iterator", we can now do iterator=True to get the underlying response iterator when downloading things.

Adapted the existing tests for the new iterator=True argument

Adapted the example for the new iterator=True argument

nejch · 2022-06-26T06:26:23Z

Thanks for all the work here @TCatshoek!

TCatshoek · 2022-06-26T10:23:16Z

@nejch Glad I could help! :D

TCatshoek mentioned this pull request Mar 30, 2022

Is there a way to get the response iterator directly when downloading artifacts? #1955

Closed

TCatshoek marked this pull request as draft March 30, 2022 21:30

nejch assigned TCatshoek Mar 31, 2022

feat(downloads): allow streaming downloads access to response iterator

4f9807f

Allow access to the underlying response iterator when downloading in streaming mode by specifying action="iterator" Update type annotations to support this change

TCatshoek force-pushed the main branch from 51ac981 to 4f9807f Compare April 9, 2022 13:50

TCatshoek added 2 commits April 9, 2022 15:52

feat(downloads): add conditional dependency on literal for python 3.7

efd8b48

This supports commit c76b3b1 and makes sure it works on python 3.7 as well.

chore(deps): add conditional dependency on typing_extensions

8cc4cfc

This is used to support Literal from typing on python < 3.8

TCatshoek added 2 commits April 10, 2022 10:16

docs(api-docs): add iterator example to artifact download

63fa05b

Document the usage of the action="iterator" option when downloading artifacts

test(packages): add tests for streaming downloads

d7ee6f8

Tests for the functionality implemented in 4f9807f

TCatshoek marked this pull request as ready for review April 10, 2022 09:00

nejch reviewed Apr 13, 2022

View reviewed changes

docs(api-docs): make iterator download documentation more generic

af3eb0b

Co-authored-by: Nejc Habjan <hab.nejc@gmail.com>

TCatshoek force-pushed the main branch from f7646f9 to af3eb0b Compare April 14, 2022 13:13

TCatshoek added 7 commits June 25, 2022 22:17

Revert "chore(deps): add conditional dependency on typing_extensions"

4b31b51

This reverts commit 8cc4cfc.

Revert "feat(downloads): add conditional dependency on literal for py…

7e5f4ee

…thon 3.7" This reverts commit efd8b48.

Revert "feat(downloads): allow streaming downloads access to response…

5a97cdb

… iterator" This reverts commit 4f9807f.

Merge remote-tracking branch 'upstream/main'

780ddc1

feat(downloads): use iterator=True for returning response iterator fo…

8580a77

…r downloads Instead of having action="iterator", we can now do iterator=True to get the underlying response iterator when downloading things.

test(packages): update tests for downloading with response iterator

ec61e73

Adapted the existing tests for the new iterator=True argument

docs(api-docs): update artifact download iterator example

9f01ee5

Adapted the example for the new iterator=True argument

nejch approved these changes Jun 26, 2022

View reviewed changes

nejch merged commit b644721 into python-gitlab:main Jun 26, 2022

	for chunk in response.iter_content(chunk_size=chunk_size):
	if chunk:
	action(chunk)

feat(downloads): allow streaming downloads access to response iterator #1956

feat(downloads): allow streaming downloads access to response iterator #1956

Uh oh!

Conversation

TCatshoek commented Mar 30, 2022

Uh oh!

codecov-commenter commented Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nejch commented Mar 31, 2022

Uh oh!

TCatshoek commented Mar 31, 2022

Uh oh!

TCatshoek commented Apr 9, 2022

Uh oh!

TCatshoek commented Apr 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nejch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nejch Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

TCatshoek Apr 14, 2022

Choose a reason for hiding this comment

Uh oh!

TCatshoek May 23, 2022

Choose a reason for hiding this comment

Uh oh!

nejch May 23, 2022

Choose a reason for hiding this comment

Uh oh!

nejch May 29, 2022

Choose a reason for hiding this comment

Uh oh!

TCatshoek May 30, 2022

Choose a reason for hiding this comment

Uh oh!

nejch May 30, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TCatshoek commented Apr 14, 2022

Uh oh!

nejch commented Jun 25, 2022

Uh oh!

TCatshoek commented Jun 25, 2022

Uh oh!

nejch commented Jun 26, 2022

Uh oh!

TCatshoek commented Jun 26, 2022

Uh oh!

Uh oh!

codecov-commenter commented Mar 31, 2022 •

edited

Loading

TCatshoek commented Apr 10, 2022 •

edited

Loading