bpo-29659: Expose copyfileobj() `length` arg for public use by goodboy · Pull Request #328 · python/cpython

goodboy · 2017-02-27T01:19:31Z

Corresponds to issue bpo-29659

When copying large files the copy time can be sufficiently
shortened by increasing the memory buffer used in the copyfileobj()
routine. Expose the length argument from copyfileobj() upwards
for use throughout the module.

https://bugs.python.org/issue29659

the-knights-who-say-ni · 2017-02-27T01:19:32Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow these steps to rectify the issue:

Sign the PSF contributor agreement. The "bugs.python.org username" requested by the form is the "Login name" field in "Your Details" at b.p.o
Wait at least one US business day and then check the "Contributor form received entry under "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

goodboy · 2017-02-27T20:03:11Z

Hello, I have completed the above steps :) Thanks! Tyler Goodlet

…

On Sun, Feb 26, 2017 at 8:19 PM, the-knights-who-say-ni < ***@***.***> wrote: Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement <https://www.python.org/psf/contrib/contrib-form/> (CLA). Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow these steps to rectify the issue: 1. Sign the PSF contributor agreement <https://www.python.org/psf/contrib/contrib-form/>. The " bugs.python.org username" requested by the form is the "Login name" field in "Your Details" at b.p.o <https://cloud.githubusercontent.com/assets/2680980/23276970/d14a380c-f9d1-11e6-883d-e13b6b211239.png> 2. *Wait at least one US business day* and then check the "Contributor form received entry under "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA) 3. Reply here saying you have completed the above steps Thanks again to your contribution and we look forward to looking at it! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#328 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARzZYMi41HaBN6XwUx3iPTjPSu_myyrks5rgiSogaJpZM4MMmcV> .

vstinner

You must update Doc/library/shutil.rst: document the new parameter, but also add a ".. versionchanged:: 3.7" section to document the change.

vstinner · 2017-03-02T17:56:22Z

Lib/shutil.py

I suggest:

if not length:
length = 16 * 1024

Sounds good!

vstinner · 2017-03-02T17:56:32Z

Lib/shutil.py

You should document the default size: 16 kB.

Will do 👍

vstinner · 2017-03-02T17:56:48Z

Lib/shutil.py

Same: document default size.

goodboy · 2017-03-03T05:26:33Z

@Haypo I believe I've addressed everything!
If there's anything I'm missing please let me know :)

vstinner · 2017-03-03T11:38:00Z

Doc/library/shutil.rst

I wouldn'd say "expose ..." but just "Added length parameter", and versionadded is not the right place to document "default is ...", put it in the documentation body.

You didn't update copyfile() signature. Same remark for the 2 other functions.

Moreover, you should also document the length in the function document, not only in versionadded. The "documentation" can just ask to refer to copyfileobj().

Shoot yeah not sure how I missed the signatures...

vstinner · 2017-03-03T11:41:16Z

Lib/shutil.py

please mention the unit, "size in bytes"

@Haypo to clarify, do you prefer:
'An in-memory buffer size can be set with length; the default size in bytes is 16*1024',
or
"An in-memory buffer size in bytes can be set with length; the default is 16 KB.
?

"An in-memory buffer size in bytes can be set withlength; the default is 16 KB."

I encourage you to use KiB, which other parts of the documentation already use. If you write 16 KB it could easily be interpreted as 16,000 B (lower-case k means 1000, as in km for kilometre).

goodboy · 2017-03-03T19:25:27Z

@Haypo hopefully 3rd times the charm ;)

If you'd like me to squash the history to one commit let me know!

terryjreedy

Suggestion: leave API of copyfileobj should alone as 'length=16*1024' to document default. For other functions, say something like 'If length not passed, copyfileobj uses its default.' We might change it someday.

Required: new or revised tests that use the expanded api (and fail without patch).

goodboy · 2017-03-04T01:42:57Z

@terryjreedy the only problem with that is that we'll need to have the if length checks in all consuming functions as opposed to only once in copyfileobj no? Maybe you have a better approach?

Yes I totally agree about the test. I guess it goes here?

vstinner · 2017-03-04T04:58:24Z

Yes I totally agree about the test. I guess it goes here?

<https://github.com/python/cpython/blob/master/Lib/test/test_shutil.py#L1900> Yes.

berkerpeksag · 2017-03-15T09:38:18Z

Doc/library/shutil.rst

Indentation should use three spaces here:

.. versionchanged:: 3.7 Added the *length* parameter.

`length` -> *length*

(and please finish the sentence with a full stop.)

When copying large files the copy time can be sufficiently shortened by increasing the memory buffer used in the `copyfileobj()` routine. Expose the `length` argument from `copyfileobj()` upwards for use throughout the module.

vadmium · 2017-11-05T22:30:24Z

Doc/library/shutil.rst

IMO “Note that” is redundant. In fact the whole first line could be dropped, and “Only the contents from the current file position . . .” could be put back in the first paragraph.

I agree with this.

vstinner

LGTM, but please address pending comments before I can approve the change ;-)

vstinner · 2017-11-06T09:35:18Z

Lib/shutil.py

vstinner · 2017-11-06T09:35:35Z

Lib/shutil.py

PEP 8: add spaces around operators, "16 * 1024"

vstinner · 2017-11-06T09:36:10Z

Lib/shutil.py

Please remove this empty line.

vstinner · 2017-11-06T09:42:57Z

Note: I created PR #4293 to replace KB with KiB in the Python code base ;-)

goodboy · 2017-11-20T01:35:11Z

I think everything has been addressed except for @terryjreedy's:

Suggestion: leave API of copyfileobj should alone as 'length=16*1024' to document default. For other functions, say something like 'If length not passed, copyfileobj uses its default.' We might change it someday.

I don't know how to keep the default value in copyfileobj but also support it as a default argument in all client functions without length = 16 * 1024 if length is None else length in each case. Personally I think that's a bit uglier. I can also stick the default value in each client function to be length=16 * 1024 and always pass it through transparently if that is preferred; it just means changing the value in multiple places if it is changed in the future. I had originally opted for one change in one place if it was to be modified later.

Whatever you all think is best I'll do 👍

vstinner · 2017-11-20T16:55:29Z

Lib/test/test_shutil.py

I suggest to use a smaller file: write('x' * 100) and then length=20.

By the way, I suggest to use tempfile.NamedTemporaryFile rather than temporary files:

with tempfile.NamedTemporaryFile() as src: with tempfile.NamedTemporaryFile() as dst: write_file(src.name, b'x' * 100, binary=True) fn(src.name, dst.name, length=20)

Hehe thanks @vstinner! You did the work for me 👍

AraHaan · 2017-11-20T21:56:47Z

Lib/shutil.py


-def copyfileobj(fsrc, fdst, length=16*1024):
-    """copy data from file-like object fsrc to file-like object fdst"""
+def copyfileobj(fsrc, fdst, length=None):


def copyfileobj(fsrc, fdst, length=16*1024):

I personally prefer this way in my code to concentrate 2 lines worth of code into one.

if not length: length = 16 * 1024

It looks a little nicer and not to mention saves lines of code that basically do the same thing.
And yes I am also guilty of doing it this way and then people started hating me for it and calling my a bad programmer for it.

goodboy · 2017-11-21T05:50:41Z

Just to clear this up since it seems my original reasoning isn't showing well and the drive-by comments are piling up.

Afaict there are 3 ways to deal with the default value of length:

stick length=16*1024 in all three function signatures for copyfileobj, copy, and copy2
- this means any time the default needs to be changed it will be done in 3 places
- this is the least amount of lines of code
keep it the way it is (i.e. copy and copy2 define length=None in their signatures and one function, copyfileobj, has the current if length is None: check)
- one more line of code then the previous solution
- have to change the length in one place in the future
- length is an explicit default argument in copy and copy2
make copy and copy2 take **kwargs and pass them through to the internal call to copyfileobj
- same lines of code as the first solution
- doesn't explicitly document length in each of the the copy, copy2 signatures
- one place to change the default value in the future

If one of these solutions is preferred over the others after acknowledging the trade-offs I'm fine to do it, but just simply saying length=16*1024 is less code doesn't deal with how to handle copy and copy2 and the extra (or less clear) code they may contain.

goodboy · 2017-11-21T05:53:15Z

@vstinner looks like the appveyor build failed due to the with open(path, 'wb' if binary else 'w') as fp: line?

vstinner · 2017-11-22T09:42:27Z

@vstinner looks like the appveyor build failed due to the with open(path, 'wb' if binary else 'w') as fp: line?

ERROR: test_copy_w_different_length (test.test_shutil.TestShutil)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\projects\cpython\lib\test\test_shutil.py", line 1417, in test_copy_w_different_length
    write_file(src.name, b'x' * 100, binary=True)
  File "C:\projects\cpython\lib\test\test_shutil.py", line 60, in write_file
    with open(path, 'wb' if binary else 'w') as fp:
PermissionError: [Errno 13] Permission denied: 'C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\tmpj1vhtx1b'

Sorry, I don't know why the test failed.

AraHaan · 2017-11-22T21:50:13Z

Looks to me like an normal permission error as python seems to not have permissions to write to the temp folder? Maybe somehow have python write to somewhere that does not require administrator permissions? It would be nice if python had an nice function that you provide the path and it checks if you have permissions to write to there to avoid having to try / except permission errors. 🤔 That function would be the perfect thing to fix this test on Windows.

goodboy · 2017-12-11T23:04:31Z

Ok so besides the outstanding:

missing news entry
failing test

Is there anything else in the patch that needs to be addressed?

AraHaan · 2017-12-11T23:31:48Z

I dont think so.

vstinner

I dislike length=-1 "feature".

vstinner · 2017-12-13T00:28:10Z

Lib/shutil.py

+    """
+    if not length:
+        length = 16 * 1024
    while 1:


I'm not sure that it's ok to use a loop if the length is negative. I suggest to have a special case for negative value calling read() (no parameter) only once.

vstinner · 2017-12-13T00:31:45Z

Doc/library/shutil.rst

+   The integer *length*, if given, is the buffer size; the default value
+   in bytes is 16 KiB. A negative *length* value means to copy the data without
+   looping over the source data in chunks; by default the data is read in
+   chunks to avoid uncontrolled memory consumption.


"A negative length value means to copy the data without looping over the source data in chunks"

I dislike this definition. In practice, negative means "unlimited" buffer size: the whole input file is loaded into memory.

I'm not sure that it's a good practice to try to load files of unknown size into memory.

I suggest to remove this feature which seems more like a side effect than a carefully designed API.

If you want to get fast copy, pass a very large length like 1 GB. But if Python starts to load 1 TB into memory, it's likely to crash the system... At least, to slow down the system, a lot.

bedevere-bot · 2017-12-13T00:32:15Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

vstinner · 2019-02-25T09:50:49Z

Fixed by https://bugs.python.org/issue33671

….6->3.7)

Bumps [gidgethub](https://github.com/brettcannon/gidgethub) from 4.0.0 to 4.1.0. - [Release notes](https://github.com/brettcannon/gidgethub/releases) - [Changelog](https://github.com/brettcannon/gidgethub/blob/master/docs/changelog.rst) - [Commits](https://github.com/brettcannon/gidgethub/commits/v4.1.0) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com> Co-authored-by: Mariatta <Mariatta@users.noreply.github.com>

the-knights-who-say-ni added the CLA not signed label Feb 27, 2017

Mariatta changed the title ~~Expose copyfileobj() length arg for public use~~ bpo-29659: Expose copyfileobj() length arg for public use Feb 27, 2017

Mariatta removed the CLA not signed label Feb 27, 2017

the-knights-who-say-ni added the CLA signed label Feb 27, 2017

vstinner requested changes Mar 2, 2017

View reviewed changes

vstinner added the type-feature A feature request or enhancement label Mar 2, 2017

goodboy force-pushed the expose_shutil_membuf-29659 branch from 4feede0 to 39a9b83 Compare March 3, 2017 05:25

vstinner requested changes Mar 3, 2017

View reviewed changes

goodboy force-pushed the expose_shutil_membuf-29659 branch from 39a9b83 to 158a71b Compare March 3, 2017 19:24

terryjreedy requested changes Mar 3, 2017

View reviewed changes

berkerpeksag reviewed Mar 15, 2017

View reviewed changes

Expose copyfileobj() length arg for public use

7fb7091

When copying large files the copy time can be sufficiently shortened by increasing the memory buffer used in the `copyfileobj()` routine. Expose the `length` argument from `copyfileobj()` upwards for use throughout the module.

goodboy force-pushed the expose_shutil_membuf-29659 branch from 158a71b to bfeaee8 Compare September 15, 2017 03:40

vadmium reviewed Nov 5, 2017

View reviewed changes

vstinner reviewed Nov 6, 2017

View reviewed changes

Document exposure of the copyfileobj length arg in shutil

24b2f57

goodboy force-pushed the expose_shutil_membuf-29659 branch from bfeaee8 to 0043566 Compare November 20, 2017 01:27

vstinner reviewed Nov 20, 2017

View reviewed changes

Add test to using new length kwarg

8daf7a8

goodboy force-pushed the expose_shutil_membuf-29659 branch from 0043566 to 8daf7a8 Compare November 20, 2017 21:51

AraHaan reviewed Nov 20, 2017

View reviewed changes

vstinner requested changes Dec 13, 2017

View reviewed changes

bedevere-bot added the awaiting changes label Dec 13, 2017

methane closed this Feb 25, 2019

klange pushed a commit to toaruos/cpython that referenced this pull request Sep 15, 2021

Issue python#28649: fix second issue with _ForwardRef (python#328) (3…

409d64a

….6->3.7)

Uh oh!

Conversation

goodboy commented Feb 27, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Feb 27, 2017

Uh oh!

goodboy commented Feb 27, 2017 via email

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goodboy commented Mar 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goodboy commented Mar 3, 2017

Uh oh!

terryjreedy left a comment

Choose a reason for hiding this comment

Uh oh!

goodboy commented Mar 4, 2017

Uh oh!

vstinner commented Mar 4, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Nov 6, 2017

Uh oh!

goodboy commented Nov 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goodboy commented Feb 27, 2017 •

edited by bedevere-bot

Loading

AraHaan Nov 20, 2017 •

edited

Loading

goodboy commented Nov 21, 2017 •

edited

Loading

AraHaan commented Nov 22, 2017 •

edited

Loading