BUG: fix the method for checking local files#23728
Conversation
|
For example: Example not work, because |
numpy/compat/py3k.py
Outdated
| def isfileobj(f): | ||
| return isinstance(f, (io.FileIO, io.BufferedReader, io.BufferedWriter)) | ||
| try: | ||
| return isinstance(f.fileno(), int) |
There was a problem hiding this comment.
I guess fileno() needs to be tried to see if it is defined, but I think this needs a test and narrowig the exception. Python defines the exception clearly here, AFAIK.
Also, I suspect this breaks e.g. if you pass in a bz2.BZ2File, so I would like to test that as well (and fix it if necessary).
There was a problem hiding this comment.
You are right. bz2.BZ2File raise error: io.UnsupportedOperation: Seeking is only supported on files open for reading. Looks like need seekable.
There was a problem hiding this comment.
I am mostly worried that the old code manages to reject file-like objects that do have a fileno() but that are also compressed. I.e. for those writing directly to the fileno() would be wrong because that would side-step compression. But even a compressed file-like might be seekable?
We have a problem with that in tofile() which fails to reject writing to such files, actually I am curious if the old isinstance check we had here is specific enough to get that right (at least in practice).
There was a problem hiding this comment.
def isfileobj(f):
if isinstance(f, (io.FileIO, io.BufferedReader, io.BufferedWriter)):
try:
return isinstance(f.fileno(), int)
except (io.UnsupportedOperation, AttributeError, NotImplementedError):
pass
return FalseThis seems more stable, although it looks a bit strange.
There was a problem hiding this comment.
Yeah, but I think it makes sense at least as a minimal fix (I do think its plausible that this should be improved more, but I am not exactly sure how).
Do we really need more than OSError there? Those classes have fileno() defined already, and as I read it are supposed to raise OSError when there is no valid fileno?
numpy/lib/format.py
Outdated
|
|
||
| """ | ||
| if isfileobj(filename): | ||
| if is_seekable_local_file(filename): |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
|
|
LoveEatCandy
force-pushed
the
bugfix/check_local_file
branch
from
7cc0187 to
9775f9d
Compare
numpy/compat/py3k.py
Outdated
|
|
||
| def isfileobj(f): | ||
| return isinstance(f, (io.FileIO, io.BufferedReader, io.BufferedWriter)) | ||
| if isinstance(f, (io.FileIO, io.BufferedReader, io.BufferedWriter)): |
There was a problem hiding this comment.
Let's remove one level of indentation via not ...:?
We should also have a mock-up test for the type of object you wish to support.
BufferedReader and BufferedWriter cannot be used to determine local files. For example, users can implement CustomFile to operate on OSS files, and then use BufferedReader(CustomFile) to achieve the buffered effect. But fileno method can do it.
LoveEatCandy
force-pushed
the
bugfix/check_local_file
branch
from
9775f9d to
a307e0f
Compare
|
I noticed that |
|
Hmmm, there should be one more 1.24.x release, which is Python 3.8 compatible. We don't backport more than one release normally (or practically ever). Maybe @charris has an opinion. For backporting to 1.24.x it may be best to add an |
|
Let me put this in, I think its good to go, thanks. Pending Chuck's opinion. If you wish a 1.24.x backport, could you do a PR against Beyond that, I guess it will just be a no, sorry. Python 3.7 is barely in security support anyway and its just not something we normally do. |
2 participants
BufferedReader and BufferedWriter cannot be used to determine local files. For example, users can implement CustomFile to operate on OSS files, and then use BufferedReader(CustomFile) to achieve the buffered effect. But fileno method can do it.