Skip to content

Fix in file position logic: file descriptor and Python-side handle #20270

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.

Sign up for GitHub

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

Jump to bottom
Closed
philippslang wants to merge 6 commits into from
Closed

Fix in file position logic: file descriptor and Python-side handle #20270

philippslang wants to merge 6 commits into from

Conversation

@philippslang
Copy link
Contributor

@philippslang philippslang commented May 8, 2019

This addresses #18436

The logic replicates the essence of closing file descriptors in numpy:
https://github.com/numpy/numpy/blob/bf20e3034085716c4559ec4bf31b23b6016f266c/numpy/core/include/numpy/npy_3kcompat.h#L278

This stores the position of the file descriptor before resetting it to the Python handle offset, then resets to the original position before exit. The Python-side handle is then updated to reflect the new position. Also added somewhat more demanding tests to cover this.

@pytorchbot pytorchbot added module: internals Related to internal abstractions in c10 and ATen module: operators module: serialization Issues related to serialization (e.g., via pickle, or otherwise) of PyTorch objects labels May 8, 2019
soumith
soumith approved these changes May 8, 2019
View reviewed changes
Copy link
Contributor

@soumith soumith left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the fix

@soumith
Copy link
Contributor

soumith commented May 8, 2019

legit build failures. Did you test this PR locally?

var/lib/jenkins/workspace/torch/csrc/generic/StorageMethods.cpp:281:68: error: 'current_pos' was not declared in this scope
May 08 12:45:28    const auto seek_return = PyObject_CallMethod(file, "seek", "li", current_pos, 0);

@philippslang
Copy link
Contributor Author

philippslang commented May 8, 2019

oh boy, renamed variables after testing

facebook-github-bot
facebook-github-bot reviewed May 9, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@soumith is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@soumith
Copy link
Contributor

soumith commented May 9, 2019

thank you!

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented May 9, 2019

@soumith merged this pull request in f23fb66.

@philippslang philippslang deleted the filedesfix branch May 9, 2019 19:20
@Roffild
Copy link
Contributor

Roffild commented Sep 27, 2019 •
edited
Loading 

 File "torch\serialization.py", line 581, in _load
    deserialized_objects[key]._set_from_file(f, offset, f_should_read_directly)
OSError: [Errno 22] Invalid argument

or bug in 5259616

@Roffild Roffild mentioned this pull request Sep 27, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

+1 more reviewer

@soumith soumith soumith approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Merged module: internals Related to internal abstractions in c10 and ATen module: serialization Issues related to serialization (e.g., via pickle, or otherwise) of PyTorch objects open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@philippslang @soumith @facebook-github-bot @Roffild @ezyang @pytorchbot @mruberry