labscript-suite / labscript-utils

Shared modules used by the 𝘭𝘒𝘣𝘴𝘀𝘳π˜ͺ𝘱𝘡 𝘴𝘢π˜ͺ𝘡𝘦. Includes a graphical exception handler, debug tools, configuration management, cross platform filepath conversions, unit conversions and custom GUI widgets.
http://labscriptsuite.org
Other
2 stars 45 forks source link

Make setup_logging() more Jupyter-friendly. #78

Closed zakv closed 2 years ago

zakv commented 3 years ago

It can be helpful to import code from labscript programs into Jupyter notebooks, particularly for development and debugging purposes. For example, I'm currently using the runviewer.__main__.Shot class to generate some plots. However, with the current version of labscript_utils, running from runviewer.__main__ import Shot from a Jupyter notebook fails, so the code cannot be imported. In the past I've seen this issue when importing some other labscript components as well.

The import fails because importing runviewer.__main__ calls labscript_utils.setup_logging.setup_logging(), which in turn raises an UnsupportedOperation error (example traceback below). The issue is that Jupyter kernels do some special things with stdout and stderr, which means that sys.stdout.fileno and sys.stderr.fileno exist but aren't callable in a Jupyter notebook. This PR changes setup_logging() to catch the UnsupportedOperation error thrown when it is run (possibly indirectly) from a Jupyter notebook. When the error is caught, logging output isn't directed to stdout or stderr, but is still sent to the log files. Note that setting sys.stdout = sys.stderr = open(os.devnull, 'w') isn't a good idea in this case because it prevents print() statements (and likely other output as well) from working in the Jupyter notebook.

This might be too niche of a problem to be worth merging. That said, it would be nice to be able to work on labscript code in a jupyter notebook and I have run into this issue multiple times.

Example traceback:

---------------------------------------------------------------------------
UnsupportedOperation                      Traceback (most recent call last)
<ipython-input-1-a4e555dfbf6f> in <module>
----> 1 from runviewer.__main__ import Shot as RVShot

c:\users\username\software\labscript\runviewer\runviewer\__main__.py in <module>
     37 splash.update_text('importing labscript suite modules')
     38 from labscript_utils.setup_logging import setup_logging
---> 39 logger = setup_logging('runviewer')
     40 labscript_utils.excepthook.set_logger(logger)
     41 

c:\users\username\software\labscript\labscript-utils\labscript_utils\setup_logging.py in setup_logging(program_name, log_level, terminal_level, maxBytes, backupCount)
     53     handler.setLevel(log_level)
     54     logger.addHandler(handler)
---> 55     if sys.stdout is not None and sys.stdout.fileno() >= 0:
     56         stdout_handler = logging.StreamHandler(sys.stdout)
     57         stdout_handler.setFormatter(formatter)

UnsupportedOperation: fileno
zakv commented 3 years ago

I also came across this issue here: https://github.com/labscript-suite/blacs/pull/88#issuecomment-774525046 (see the note at the end of that comment)

dihm commented 2 years ago

I actually really like this change and think it should be merged. I, too, like to develop code using jupyter notebooks. Being able to do that with labscript code as well would be appreciated. If nothing else, I know of a few labscript devices that ship with their own drivers so to even test the drivers you have to be able to import labscript. I think this alone makes this a fairly necessary addition.

I would suggest that some sort of warning gets emitted somewhere that signifies this is happening. The concern I have is this catches a fairly generic error just to determine the very specific case that code is being run from jupyter. What if that error is emitted for something unrelated and this silently kills logging to the stdout? I think adding a log message saying the shell redirect has been skipped would cover that base and shouldn't make this any more complicated (hopefully, tell me if I'm wrong).

zakv commented 2 years ago

Good idea. I added a warning and also changed the try/except block into a try/except/else construct so that an UnsupportedOperation is only caught if it is raised by sys.stdout.fileno(). That should reduce the chance that this generic error is mistakenly caught.

dihm commented 2 years ago

This looks good to me, but the docs build is failing. I suspect this is because this branch diverged from main before the docs builds were updated. Could you rebase this branch onto master just to ensure nothing silly is going on?

zakv commented 2 years ago

Rebased and now the docs build!

dihm commented 2 years ago

Cool! I'm going to merge.