Navigation Menu

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

[Jupyter] Fails silently on importing C header file in same folder #3751

Closed
bergkvist opened this issue Jul 23, 2020 · 8 comments · Fixed by #3872
Closed

[Jupyter] Fails silently on importing C header file in same folder #3751

bergkvist opened this issue Jul 23, 2020 · 8 comments · Fixed by #3872

Comments

@bergkvist
Copy link

bergkvist commented Jul 23, 2020

The code below "evaluates" without any error messages. By myadd is never defined, and the annotation output is never produced.

Removing the dependency on add.h makes everything work without a problem.

File system:

src/
    - add.h
    - Notebook.ipynb

C Header File (standalone)

// add.h
int add(int x, int y) {
    return x + y;
}

Jupyter Notebook:

# Input cell [1]
%load_ext cython
# Input cell [2]
%%cython --annotate

cdef extern from "add.h":
    int add(int x, int y)

cpdef myadd(int x, int y):
    cdef int total = add(x, y)
    return total

To reproduce/use the same environment, you can use docker:

docker run --rm -it -p8888:8888 jupyter/scipy-notebook:ea01ec4d9f57
@bergkvist
Copy link
Author

Changing to the following makes it work. (Notice that I added -I. at the top)

# Input cell [2]
%%cython --annotate -I.

cdef extern from "add.h":
    int add(int x, int y)

cpdef myadd(int x, int y):
    cdef int total = add(x, y)
    return total

Would be nice if an error was thrown, saying the header file was not found. Failing silently makes it a lot harder to track down the problem.

@da-woods
Copy link
Contributor

The compile output from Jupyter output appears in the terminal that Jupyter is run in, not the notebook itself.

It would be nice if it could be redirected to the notebook (at least on failure) - it'd save a lot of confusion for people. I don't think capturing and redirecting it is particularly easy though.

@da-woods
Copy link
Contributor

da-woods commented Jul 24, 2020

Duplicate of #1569

Try with the --verbose option and it should be more obvious

@da-woods
Copy link
Contributor

da-woods commented Jul 24, 2020

On closer inspection the verbose option doesn't look to be that much more verbose. I think intercepting the compiler output is harder than it looks. I'll reopen and someone else can decide what to do with this.

@da-woods da-woods reopened this Jul 24, 2020
@realead
Copy link
Contributor

realead commented Aug 24, 2020

I think the problem comes with #3196: While in IPython one can see things printed to c-level stdout this is not the case for Jupiter-notebooks.

There is wurlitzer (https://github.com/minrk/wurlitzer) which has the IPython-magic %load_ext wurlizer to capture C-level stdout for Jupiter-notebooks, sadly it works only for Linux (and probably macOS), but this is probably not the way to go.

Another problem with #3196 is that while it catches distutils.errors.CompileError it doesn't catch the distutils.errors.LinkError, for which one still sees the old message.

Maybe a better solution would be to catch both exceptions, but also to print to sys.stderr (which for Jupiter-notebook has nothing to do with C-level stderr) that an error has happened while compiling/linking and the user should see logs on C-level stderr/stdout?

@scoder
Copy link
Contributor

scoder commented Oct 2, 2020

Isn't it enough to divert sys.stderr before running the extension build?
That obviously introduces concurrency problems if we're not the only thread creating output at this time, but that case should be fairly rare, at least for most users.

Everything beyond that, I'd say, should be improved on the side of setuptools, which controls the C compiler run.

@realead
Copy link
Contributor

realead commented Oct 2, 2020

@scoder

Isn't it enough to divert sys.stderr before running the extension build?

Not sure I understand that, I think redirecting sys.stderr will not change where the C-level output will be written to?

When compiler (or linker) is called by distutils, the output of the compiler/linker isn't captured (see here):

    try:
        proc = subprocess.Popen(cmd, env=env)
        proc.wait()
        exitcode = proc.returncode

and goes directly to the C-level stdout/stderr bypassing the whole python-IO-machinery. In normal python/ipython session we still see linker/gcc-output, because it is the same terminal. However in a jupyter-notebook, only things printed to python's stdout/stderr are rendered in the browser, not the things printed directly to C-level stdout/stderr (which are seen in the terminal from which the jupyter was started).

This is the reason why right now, even if compile step failed, a Jupyter user assumes, that everything worked as expected and sees no feedback, that the building the extension failed, see #3840

  1. The best way is probably to catch stderr/stdout while calling compiler/linker in distutils and redirect this to python's stderr/stdout (not sure why this wasn't done in the first place). Not sure whether this solution makes sense or is feasible any time soon.

  2. First workaround: One could catch the C-level output while executing %%cython, and redirect it to sys.stderr. There is wurlitzer, which does that (based on this article), but its approach with ctypes doesn't work on Windows (at least not out-of-the box, I also wasn't able to make it work, but maybe it is just me). However, it is possible to write a C-extension, which would work for Linux and Windows. I have written a prototype with Cython some time ago (https://github.com/realead/coutcatcher), not sure it is waterproof tough.

  3. Minimal workaround (at core of PR Fix silent cython-magic build failure on jupyter-notebooks #3819): notify the user, that the build didn't succeed, which gives a hint to look for the output in terminal.

Even if I bug python-devs with 1) now, it will take time (if it will be considered a valid concern) to be fixed/improved. Until then 3) would be not ideal, but at least an improvement compared to the current situation.

What would be a good solution from your point of view? I'll try my best to implement it and to provide a PR to solve this problem.

@scoder
Copy link
Contributor

scoder commented Oct 2, 2020

In our test runner, we do this:

cython/Cython/Utils.py

Lines 396 to 433 in af75799

def captured_fd(stream=2, encoding=None):
pipe_in = t = None
orig_stream = os.dup(stream) # keep copy of original stream
try:
pipe_in, pipe_out = os.pipe()
os.dup2(pipe_out, stream) # replace stream by copy of pipe
try:
os.close(pipe_out) # close original pipe-out stream
data = []
def copy():
try:
while True:
d = os.read(pipe_in, 1000)
if d:
data.append(d)
else:
break
finally:
os.close(pipe_in)
def get_output():
output = b''.join(data)
if encoding:
output = output.decode(encoding)
return output
from threading import Thread
t = Thread(target=copy)
t.daemon = True # just in case
t.start()
yield get_output
finally:
os.dup2(orig_stream, stream) # restore original stream
if t is not None:
t.join()
finally:
os.close(orig_stream)

Admittedly, that's a bit involved, but it gets the job done.

Note that the people to approach for 1) are not the CPython devs but the setuptools devs. The distutils package in the standard library has been deprecated and development has moved to setuptools. See my link above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants