NotificationsYou must be signed in to change notification settings
Fork673
Star2.4k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nejch merged 13 commits intopython-gitlab:mainfromTCatshoek:main

Jun 26, 2022

Merged

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

nejch merged 13 commits intopython-gitlab:mainfromTCatshoek:main

Jun 26, 2022

Conversation

Copy link

Contributor

TCatshoek commentedMar 30, 2022

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

Also update type annotations to support this change

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

Copy link

codecov-commenter commentedMar 31, 2022•
edited
Loading

Codecov Report

Merging#1956 (9f01ee5) intomain (f9b7c7b) willincrease coverage by0.00%.
The diff coverage is92.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%> (ø)`

Copy link

Member

nejch commentedMar 31, 2022

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 onhttps://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 withaction="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 in5a1678f

	deftest_download_generic_package(project):
	package=project.generic_packages.download(
	package_name=package_name,
	package_version=package_version,
	file_name=file_name,
	)

	assertisinstance(package,bytes)
	assertpackage.decode("utf-8")==file_content


	deftest_download_generic_package_to_file(tmp_path,project):
	path=tmp_path/file_name

	withopen(path,"wb")asf:
	project.generic_packages.download(
	package_name=package_name,
	package_version=package_version,
	file_name=file_name,
	streamed=True,
	action=f.write,
	)

	withopen(path,"r")asf:
	assertf.read()==file_content

nejch assignedTCatshoek

Mar 31, 2022

Copy link

ContributorAuthor

TCatshoek commentedMar 31, 2022

@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

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

4f9807f

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

TCatshoek force-pushed themain branch from51ac981 to4f9807fCompare

April 9, 2022 13:50

TCatshoek added2 commits

April 9, 2022 15:52

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

efd8b48

This supports commitc76b3b1 and makessure 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

Copy link

ContributorAuthor

TCatshoek commentedApr 9, 2022

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!

TCatshoek added2 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 downloadingartifacts

test(packages): add tests for streaming downloads

d7ee6f8

Tests for the functionality implemented in4f9807f

TCatshoek marked this pull request as ready for review

April 10, 2022 09:00

Copy link

ContributorAuthor

TCatshoek commentedApr 10, 2022•
edited
Loading

Added tests and documentation too!

nejch reviewed

Apr 13, 2022

View reviewed changes

Copy link

Member

nejch left a comment

Choose a reason for hiding this comment

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

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 OutdatedShow resolvedHide resolved

requirements.txt Outdated

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

Copy link

Member

nejchApr 13, 2022

Choose a reason for hiding this comment

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

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

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

Copy link

ContributorAuthor

TCatshoekApr 14, 2022

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

TCatshoekMay 23, 2022

Choose a reason for hiding this comment

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

@nejch Any news on this?

Copy link

Member

nejchMay 23, 2022

Choose a reason for hiding this comment

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

Thanks for your patience@TCatshoek!

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

python-gitlab/gitlab/utils.py

Lines 44 to 46 in792cee9

	forchunkinresponse.iter_content(chunk_size=chunk_size):
	ifchunk:
	action(chunk)

So I think that might be a bit confusing. How about an explicititerator: bool = False argument that would follow thestreamed 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.

Copy link

Member

nejchMay 29, 2022

Choose a reason for hiding this comment

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

Just had some discussion with@JohnVillalovos and we're thinking of intruducing aniterator=True for ourlist() calls where we currently useas_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 withiterator is the way to go,

Copy link

ContributorAuthor

TCatshoekMay 30, 2022

Choose a reason for hiding this comment

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

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

Copy link

Member

nejchMay 30, 2022

Choose a reason for hiding this comment

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

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

gitlab/v4/objects/snippets.py OutdatedShow resolvedHide resolved

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

af3eb0b

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

TCatshoek force-pushed themain branch fromf7646f9 toaf3eb0bCompare

April 14, 2022 13:13

Copy link

ContributorAuthor

TCatshoek commentedApr 14, 2022

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

Copy link

Member

nejch commentedJun 25, 2022

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

Copy link

ContributorAuthor

TCatshoek commentedJun 25, 2022

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

TCatshoek added7 commits

June 25, 2022 22:17

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

4b31b51

This reverts commit8cc4cfc.

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

7e5f4ee

…thon 3.7"This reverts commitefd8b48.

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

5a97cdb

… iterator"This reverts commit4f9807f.

Merge remote-tracking branch 'upstream/main'

780ddc1

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

8580a77

…r downloadsInstead 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

Copy link

Member

nejch commentedJun 26, 2022

Thanks for all the work here@TCatshoek!

nejch merged commitb644721 intopython-gitlab:main

Jun 26, 2022

Copy link

ContributorAuthor

TCatshoek commentedJun 26, 2022

@nejch Glad I could help! :D

Labels

None yet

3 participants

Movatterモバイル変換

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

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

Uh oh!

Conversation

TCatshoek commentedMar 30, 2022

Uh oh!

codecov-commenter commentedMar 31, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nejch commentedMar 31, 2022

Uh oh!

TCatshoek commentedMar 31, 2022

Uh oh!

TCatshoek commentedApr 9, 2022

Uh oh!

TCatshoek commentedApr 10, 2022• editedLoading 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!

nejchApr 13, 2022

Choose a reason for hiding this comment

Uh oh!

TCatshoekApr 14, 2022

Choose a reason for hiding this comment

Uh oh!

TCatshoekMay 23, 2022

Choose a reason for hiding this comment

Uh oh!

nejchMay 23, 2022

Choose a reason for hiding this comment

Uh oh!

nejchMay 29, 2022

Choose a reason for hiding this comment

Uh oh!

TCatshoekMay 30, 2022

Choose a reason for hiding this comment

Uh oh!

nejchMay 30, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TCatshoek commentedApr 14, 2022

Uh oh!

nejch commentedJun 25, 2022

Uh oh!

TCatshoek commentedJun 25, 2022

Uh oh!

nejch commentedJun 26, 2022

Uh oh!

TCatshoek commentedJun 26, 2022

Uh oh!

Uh oh!

codecov-commenter commentedMar 31, 2022•
edited
Loading

TCatshoek commentedApr 10, 2022•
edited
Loading