ENH: Allow reading from buffered stdout using np.fromfile #12324

hmaarrfk · 2018-11-04T06:48:08Z

Previously, np.fromfile would check if the object was an object of RawIO. If it was, then it wasn't seekable.

Unfortunately, (at least in python 3) this doesn't work for stdout as it is buffered but not seekable (at least on linux).

This fixes that by calling file.seekable()

Tests are also added to ensure the behaviour lives on.

Fixes #12309

Patch from 2018, before rebase in 2022:
12324_before_rebase.patch.txt

numpy/core/include/numpy/npy_3kcompat.h

hmaarrfk · 2018-11-04T12:53:45Z

Shippable failed with this test:

=================================== FAILURES ===================================
______________________ TestPathUsage.test_tofile_fromfile ______________________
[gw0] linux2 -- Python 2.7.12 /root/venv/2.7/bin/python

self = <numpy.core.tests.test_records.TestPathUsage object at 0xffff8d904f90>
    def test_tofile_fromfile(self):
        with temppath(suffix='.bin') as path:
            path = Path(path)
            a = np.empty(10, dtype='f8,i4,a5')
            a[5] = (0.5,10,'abcde')
            a.newbyteorder('<')
            with path.open("wb") as fd:
                a.tofile(fd)
            x = np.core.records.fromfile(path,
                                         formats='f8,i4,a5',
                                         shape=10,
                                         byteorder='<')

but I can't seem to find it when i search...

hmaarrfk · 2018-11-04T13:03:29Z

Is there a preference between

ret = PyObject_CallMethod(file, "seekable", "");

and

ret = PyObject_CallMethod(file, "seekable", NULL);

numpy/core/include/numpy/npy_3kcompat.h

eric-wieser · 2018-11-05T00:02:47Z

I'd probably opt for passing NULL - I'd expect it to be faster, else CPython wouldn't bother supporting it.

hmaarrfk · 2018-11-05T00:04:11Z

Ok.

Any clue where the python 2.7 test is? Seems important...

eric-wieser · 2018-11-05T00:05:26Z

numpy.core.tests.test_records would be at numpy/core/tests/test_records.py

charris · 2018-11-05T00:12:02Z

Shippable failed with this test:

I've seen that failure on other PRs, but not all, or even all recent, so I don't know what is the cause. It seems inconsistent, only on shippable, and only on Python 2.7. Might try a rebase to see if that fixes it.

hmaarrfk · 2018-11-05T00:14:16Z

Sorry my repo was out of sync.

How is it even getting in that test? Path should be None.....

I rebased, lets see what happens.

numpy/core/include/numpy/npy_3kcompat.h

numpy/core/tests/test_longdouble.py

tylerjereddy · 2018-11-05T00:25:44Z

The shippable CI uses pathlib for 2.x backporting of the functionality. I guess we could try switching to pathlib2, which is perhaps more well maintained. Or just accept not using pathlib on 2.x & remove that from shippable 2.7 job.

hmaarrfk · 2018-11-05T00:27:14Z

Thanks @tylerjereddy.

Is this considered a bugfix or feature? If it is a feature, then maybe we can just ignore 2.7?

hmaarrfk · 2018-11-05T00:36:55Z

@charris didn't mean to ignore your comment. Thanks for chiming in. my webpage was out of sync.

tylerjereddy · 2018-11-05T00:40:11Z

I opened another PR to try to fix the shippable 2.x issue. In that PR & here I see circleci now starting to fail for some reason!

eric-wieser · 2018-11-05T01:09:21Z

How is it even getting in that test? Path should be None.....

I can't see the old shippable output, as the link is discarded every time you rebase - so can't help you. Can you link to the failed output next time?

Edit: https://app.shippable.com/github/numpy/numpy/runs/1301/1/tests, I assume

hmaarrfk · 2018-11-05T01:11:50Z

@eric-wieser it seems that shippable is using a backport of pathlib, I didn't know that. Hence the discussion in #12330

hmaarrfk · 2018-11-05T01:29:39Z

@eric-wieser adding te comment here since that subthread is annoying.

On the line:

numpy/numpy/core/include/numpy/npy_3kcompat.h

Line 220 in 619926d

PyErr_SetString(PyExc_IOError,

It detects an error, but keeps going. Shouldn't it return in the if statement?

numpy/core/include/numpy/npy_3kcompat.h

hmaarrfk · 2018-11-05T01:39:54Z

I would also like it if a out parameter was provided to fromfile does that discussion go to the the mailing list, or issue tracker?

eric-wieser · 2018-11-05T02:02:57Z

It detects an error, but keeps going. Shouldn't it return in the if statement?

Congrats, you found a bug

hmaarrfk · 2018-11-07T15:22:21Z

@eric-wieser It seems that the code that tries to guess how long a file is also uses seek, but doesn't check the seekable parameter. It seems difficult to tell the size of the stdout buffer otherwise.

Basically multiarraymodule.c should check the output of originpos and nin somewhere in:

    fp = npy_PyFile_Dup2(file, "rb", &orig_pos);
    if (fp == NULL) {
        Py_DECREF(file);
        return NULL;
    }
    if (type == NULL) {
        type = PyArray_DescrFromType(NPY_DEFAULT_TYPE);
    }
    ret = PyArray_FromFile(fp, type, (npy_intp) nin, sep);

and return a resonable error message telling the user that they must specify count for unseekable file buffers. Maybe I'll get to it this weekend.

In my mind, a patch would include a test that raises an exception when the count parameter isn't provided when trying to read from the stdout buffer.

hmaarrfk · 2018-11-07T20:08:24Z

Thanks eric, seems like there is a serious bug that was found due to the tests you requested. I'll have to keep looking into it.

numpy/core/include/numpy/npy_3kcompat.h

hmaarrfk · 2018-11-10T17:00:08Z

@eric-wieser i'm stumped. Basically, it works in pure python as shown by the tests

https://travis-ci.org/numpy/numpy/jobs/453301883#L4540

only 2 "numpy-ic" fail, the two "pythonic" ones pass well. I split the tests so you can do it on your computer by checking out the commit before the mods to the implementation are done.

My ultimate goal here would be to make a well tested fromfile so that I can expand the capabilities with an out parameter.

I couldn't find many tests for fromfile 😕

mattip · 2024-07-03T12:03:47Z

@hmaarrfk would you like to try to revive this?

hmaarrfk · 2024-07-03T12:58:26Z

Thanks mat. While I don't need this anymore, I think that it is a good addition.

I'm not sure why the needs-tests label was added. I thought I had added enough tests in 2018 with the help of Eric.

Honestly, if more tests are needed, I don't think I"ll be able to be in the right headspace to craft them.

hmaarrfk · 2024-07-03T13:00:55Z

osx:

2024-07-03T12:57:10.0060650Z >           arr = np.fromfile(p.stdout, dtype=np.uint8, count=len(s1))
2024-07-03T12:57:10.0061030Z E           OSError: [Errno 29] Illegal seek

hmaarrfk · 2024-07-03T13:23:18Z

Unfortunately, these test failures are not ones I have an intention to debug anymore.

This can be used as a reference in case anybody else wants to implement this feature in the future (perhaps even myself).

mattip · 2024-07-03T16:50:29Z

Thanks for giving it a refresh, sorry it didn't pan out.

eric-wieser reviewed Nov 4, 2018

View reviewed changes