Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix HDF5Serializer test for h5py<2.9 #8220

Merged
merged 2 commits into from Oct 3, 2019

Conversation

toslunar
Copy link
Member

@toslunar toslunar commented Oct 2, 2019

@toslunar toslunar added cat:test Test or CI related. to-be-backported Pull request that should be backported. labels Oct 2, 2019
@niboshi
Copy link
Member

niboshi commented Oct 2, 2019

How about using tempfile.NamedTemporaryFile?

import os
import tempfile


with tempfile.NamedTemporaryFile() as f:
    with open(f.name, 'w') as f2:
        f2.write('aa\n')
        f2.write('bb\n')

    with open(f.name, 'r') as f3:
        print(f3.readlines())

print(os.path.exists(f.name))
['aa\n', 'bb\n']
False

@niboshi niboshi added this to the v7.0.0rc1 milestone Oct 3, 2019
@niboshi niboshi self-assigned this Oct 3, 2019
@toslunar
Copy link
Member Author

toslunar commented Oct 3, 2019

Is the code guaranteed to run?

Whether the name can be used to open the file a second time, while the named temporary file is still open, varies across platforms (it can be so used on Unix; it cannot on Windows NT or later).

https://docs.python.org/3/library/tempfile.html#tempfile.NamedTemporaryFile

In any case, I'll add the comment to the code about which tempfile API should be used after the discussion is settled.

@niboshi
Copy link
Member

niboshi commented Oct 3, 2019

@toslunar
OK, it seems NamedTemporaryFile cannot be used.

@niboshi
Copy link
Member

niboshi commented Oct 3, 2019

LGTM
Jenkins, test this please

@niboshi niboshi added the st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. label Oct 3, 2019
@niboshi niboshi changed the title Make #8182 work with h5py<2.9 Fix HDF5Serializer test for h5py<2.9 Oct 3, 2019
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 88041a8, target branch master) succeeded!

@mergify mergify bot merged commit cfaa105 into chainer:master Oct 3, 2019
@toslunar toslunar deleted the fix-8182-h5py-compat branch October 3, 2019 10:27
@chainer-ci
Copy link
Member

@mergify[bot] This pull-request is marked as to-be-backported, but the corresponding backport PR could not be found. Could you check?

toslunar pushed a commit to toslunar/chainer that referenced this pull request Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:test Test or CI related. st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. to-be-backported Pull request that should be backported.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants