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

Try to capture all filedescriptor output and err #630

Merged
merged 8 commits into from
Apr 20, 2021

Conversation

Carreau
Copy link
Member

@Carreau Carreau commented Apr 6, 2021

This try to fix a long standing issue that stdout and stderr going
directly to the filedescriptor are not shown in notebooks.

This is annoying when using wrappers around c-libraries, or calling
system commands as those will not be seen from within notebook.

Here we redirect and split the filedescriptor and watch those in threads
and redirect both to the original FD (terminal), and ZMQ (notebook).
Thus output sent to fd 1 & 2 will be shown BOTH in terminal that
launched the notebook server and in notebook themselves.

One of the concern is that now logs and errors internal to ipykernel may
appear in the notebook themselves, so may confuse user; though these
should be limited to error and debug; and we can workaround this by
setting the log handler to not be stdout/err.

This still seem like a big hack to me, and I don't like thread.

I did not manage to make reading the FD non-blocking; so this cannot be
put in the io-thread – at least I'm not sure how. So adds 2 extra
threads to the kernel.

This might need to be turn off by default for now until further testing.

Locally this seem to work with things like:

  • os.system("echo HELLO WORLD")
  • c-extensions writing directly to fd 1 and 2 (when properly flushed).

I have no clue how filedescriptor work on windows, so this only change
behavior on linux and mac.

@mlucool
Copy link
Contributor

mlucool commented Apr 7, 2021

This looks great!

One more edge case that may be something to cover here or in a separate PR:

subprocess.run(['python', '-c', 'import os; os.system("echo HELLO WORLD"); print("Hello again")'], stderr=sys.stderr)
UnsupportedOperation: fileno
---------------------------------------------------------------------------
UnsupportedOperation                      Traceback (most recent call last)
/tmp/ipykernel_100184/3677878195.py in <module>
----> 1 subprocess.run(['python', '-c', 'import os; os.system("echo HELLO WORLD"); print("Hello again")'], stderr=sys.stderr)

/opt/python/python-3.7/lib64/python3.7/subprocess.py in run(input, capture_output, timeout, check, *popenargs, **kwargs)
    470         kwargs['stderr'] = PIPE
    471 
--> 472     with Popen(*popenargs, **kwargs) as process:
    473         try:
    474             stdout, stderr = process.communicate(input, timeout=timeout)

/opt/python/python-3.7/lib64/python3.7/subprocess.py in __init__(self, args, bufsize, executable, stdin, stdout, stderr, preexec_fn, close_fds, shell, cwd, env, universal_newlines, startupinfo, creationflags, restore_signals, start_new_session, pass_fds, encoding, errors, text)
    726         (p2cread, p2cwrite,
    727          c2pread, c2pwrite,
--> 728          errread, errwrite) = self._get_handles(stdin, stdout, stderr)
    729 
    730         # We wrap OS handles *before* launching the child, otherwise a

/opt/python/python-3.7/lib64/python3.7/subprocess.py in _get_handles(self, stdin, stdout, stderr)
   1374             else:
   1375                 # Assuming file-like object
-> 1376                 errwrite = stderr.fileno()
   1377 
   1378             return (p2cread, p2cwrite,

UnsupportedOperation: fileno

@Carreau
Copy link
Member Author

Carreau commented Apr 12, 2021

I've added a fileno() to the output streams sothe issue you pointed out with subprocess() should be fixed.

I'd like to at least get a thumb up from @SylvainCorlay or someone of the xeus-python team, I'm not sure how they manipulate stderr/out and if that affects them.

Test will be incomplete as pytest seem to setup its own redirection to capture stderr/out which messes up our redirection.

This try to fix a long standing issue that stdout and stderr going
directly to the filedescriptor are not shown in notebooks.

This is annoying when using wrappers around c-libraries, or calling
system commands as those will not be seen from within notebook.

Here we redirect and split the filedescriptor and watch those in threads
and redirect both to the original FD (terminal), and ZMQ (notebook).
Thus output sent to fd 1 & 2 will be shown BOTH in terminal that
launched the notebook server and in notebook themselves.

One of the concern is that now logs and errors internal to ipykernel may
appear in the notebook themselves, so may confuse user; though these
should be limited to error and debug; and we can workaround this by
setting the log handler to not be stdout/err.

This still seem like a big hack to me, and I don't like thread.

I did not manage to make reading the FD non-blocking; so this cannot be
put in the io-thread – at least I'm not sure how. So adds 2 extra
threads to the kernel.

This might need to be turn off by default for now until further testing.

Locally this seem to work with things like:

  - os.system("echo HELLO WORLD")
  - c-extensions writing directly to fd 1 and 2 (when properly flushed).

I have no clue how filedescriptor work on windows, so this only change
behavior on linux and mac.
Bypass the original fd 2 for stderr, and use the new piped one.
This is important for example for subprocess that will peak at the
filedescriptor.
@SylvainCorlay
Copy link
Member

Thank you for pinging me @Carreau. @martinRenou was looking into something along the same lines, just when you posted your Pull Request.

Let's look more into this.

@blink1073 blink1073 added this to the 6.0 milestone Apr 19, 2021
@Carreau
Copy link
Member Author

Carreau commented Apr 20, 2021

Let's merge to see if we get issues on master; feel free to revert and merge the revert if you encounter andy issues.

@Carreau Carreau merged commit 91b935f into ipython:master Apr 20, 2021
@SylvainCorlay
Copy link
Member

That's how you do it :)

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

Successfully merging this pull request may close these issues.

4 participants