ARROW-2036: [Python] Support standard IOBase methods on NativeFile

[jcrist]

Adds support for most common file methods, adding enough to use
io.TextIOWrapper.

Added attribtes/methods:

• closed attribute
• readable, writable, seekable methods
• read1 alias for read to support TextIOWrapper on python 2
• No-op flush method

Also refactored the cython internals a bit, adding default settings for
is_readable and is_writable, which makes subclasses not need to set
them in all places.

Also renamed is_writeable to is_writable for common spelling with
the standard python method writable.

[jcrist]

Also refactored the cython internals a bit, adding default settings for
is_readable and is_writable, which makes subclasses not need to set
them in all places.

I believe this also fixes https://issues.apache.org/jira/browse/ARROW-2030, cc @cpcloud.

edit: I may have misunderstood ARROW-2030, this may not apply here

[cpcloud]

@jcrist I don't think this PR fixes that because even if the initialization happens in the parent class it still won't show up in the child instance. If you attach public or readonly then the child sees the initialized values of the parent's parameters. There's some aspect of cdef class initialization that's not 100% clear to me.

[jcrist]

@jcrist I don't think this PR fixes that because even if the initialization happens in the parent class it still won't show up in the child instance.

Are you sure? I added a bogus attribute (without readonly or public), set it only in the __cinit__ of the base class, and added a method in a subclass that accesses it. Everything worked fine.

[jcrist]

Should own_file be true for most of these? I stuck with the existing behavior, but this seems odd to me. For example MemoryMappedFile opens the file itself, why isn't it responsible for closing it on __dealloc__? Are we relying on a C++ destructor to handle that?

[wesm]

I think this arose due to some wacky GC issues with HDFS file handles, I'd have to look back at the original commit that added this to remember. My prior would be that __dealloc__ / __del__ should close the file if it is open. With MemoryMappedFile, this is a no-op because the file descriptor is owned by the mapped buffer, whose lifetime may exceed the file object

[jcrist]

Ok. I'll leave that for a follow-up PR if that's ok. I believe this attribute is unnecessary for any of the existing file objects.

[cpcloud]

@jcrist

In [12]: import pyarrow as pa

In [13]: hdfsfile = pa.HdfsFile()

In [14]: hasattr(hdfsfile, 'is_readable')
Out[14]: False

[cpcloud]

If you look at the generated C code, there's no PyMemberDef array generated to enable attribute lookup in the interpreter. You can access attributes in Cython code because it's not going through Python's attribute access mechanism

[jcrist]

That's a completely separate issue than the one described though. Those attributes are C level only, not exposed to python. This has nothing to do with initialization order or subclassing.

FWIW this PR adds readable/writable methods to mirror the standard python file interface, which are thin wrappers around those attributes. I don't think is_readable/is_writable should be exposed at the Python level, as they're part of the implementation and not standard python file attributes.

[cpcloud]

I am aware that those properties are C only.

ARROW-2030 was showing up in a user's code as an attribute error (IIRC not because they were explicitly accessing the property) which occurred because something was calling _assert_readable() and raising an exception.

It's not about making them accessible, it's about the manifestation of a possible bug.

It's possible I'm mistaken here, so I'll post in the JIRA with a reproducible test case if I can make one. We can continue the discussion there.

[jcrist]

Sounds good, I suspect we're just talking past each other here. Apologies for sidetracking this PR.

In regards to this PR, I'm a bit unsure about the writeable -> writable change. If you google "writeable or writable" you'll find other projects(e.g. nodejs) that have also gone through this decision - most settled on writable as it's a more common english spelling. I made this change originally to mirror the added writable method (part of the python file interface). However, a few dependencies of pyarrow (libhdfs and numpy) use "writeable" instead, as does the arrow c++ code. I could go either way on this change.

[wesm]

I renamed Writeable -> Writable in C++ recently, so probably best to use the more common spelling such that it's not disruptive

[jcrist]

I renamed Writeable -> Writable in C++ recently, so probably best to use the more common spelling such that it's not disruptive

Was this supposed to be a complete port over from "writeable" to "writable" (minus libhdfs, which is a dependency)? I found a few classes/methods still using "Writeable" (e.g. WriteableFile or OSFile::OpenWriteable).

---

The python code uses writable wherever it makes sense. I think this is ready for review/merge.

[pitrou]

Unrelated, but I wonder what happens if two threads call close() concurrently.

[jcrist]

Hmmm, maybe the nogil block here should be removed?

[pitrou]

Well, if rd_file.close() or wr_file.close() may do I/O, you want to release the GIL.
One possible solution is something like:

rd_file, self.rd_file = self.rd_file, None
wr_file, self.wr_file = self.wr_file, None
with nogil:
    if self.is_readable:
        check_status(rd_file.get().close())
    else:
        check_status(rd_file.get().close())

[jcrist]

That'd work, but prevents calling close again in the case of a failure. It's not actually clear to me what should happen if close fails (reset state?, set closed anayway?). Anyway, should probably open a jira about this, as it's unrelated to this PR.

[pitrou]

A general guideline for close() is that even if it fails somehow (e.g. IO error), it should still put the object in a "closed" state (and release associated resources).

[wesm]

GIL should be released here -- if any Python resources are required in Close then the GIL will be re-acquired

[jcrist]

Test failure is unrelated, ready for review. We're waiting on this to simplify a PR adding arrow's hdfs (really just libhdfs) support to dask.

[wesm]

Looking now. Can you rebase?

[wesm]

This looks fine -- just a minor comment about methods that might be better as properties (@cpcloud do you have an opinion?)

[wesm]

Should these be properties?

[jcrist]

The seekable/readable/writable? No, this is required for the python file interface. See https://docs.python.org/3/library/io.html#io.IOBase

[wesm]

Roger, thanks

[wesm]

GIL should be released here -- if any Python resources are required in Close then the GIL will be re-acquired

[jcrist]

Can you rebase?

Rebased. I'm curious why was that necessary; there were no merge conflicts (none of the files edited have been touched since I submitted the PR). Just to run the CI again?

[wesm]

@jcrist the build was failing due to a Parquet dependency upgrade (Thrift 0.11), so the Python tests wouldn't run without picking up recent patches 0621765

[wesm]

+1, will merge once the build completes

[wesm]

@jcrist can you enable Appveyor on your fork? Then builds for your PRs will run a little more quickly there

[jcrist]

Done: https://ci.appveyor.com/project/jcrist/arrow

[wesm]

Cool, from https://ci.appveyor.com/project/jcrist/arrow/build/1.0.1 looks like we are good. The Travis CI failure is ARROW-2062