You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Fixes#1951. The problem where logging messages were shown in the incorrect cell (the cell where pint.logging.setup() was called) seems to have been caused by passing the enqueue=True option to Loguru's logger.add() function. This PR simply removes the option. I've confirmed that this works both in a newer environment that showed the issue, and in an older one, which didn't have this issue, even with enqueue=True.
I'm not sure why we were using enqueue=True to begin with. The docstring for logger.add() says this about it:
enqueue : |bool|, optional
Whether the messages to be logged should first pass through a multiprocessing-safe queue
before reaching the sink. This is useful while logging to a file through multiple
processes. This also has the advantage of making logging calls non-blocking.
Hopefully getting rid of this option (and switching back to enqueue=False, which is the default in Loguru) will not cause issues in some use case I'm not aware of involving multiple PINT processes trying to log to a single file. But I think most likely this option was originally added in an attempt to get logging in a Jupyter notebook to work better, which is now backfiring.
The test failure here is happening for MacOS only, so I can't easily reproduce it. It doesn't seem to be directly related to the changes here; it might be that the current master branch version would fail in the same way.
The problem seems to be a bunch of cases of TypeError: cannot pickle 'EncodedFile' instances in test_grid.py.
Edit: On second thought, it might be that this is related to the changes here, because test_grid.py is doing some stuff involving inter-process communication, and it might be that using enqueue=False is making it fall back to some method involving pickling. If so, that's unfortunate, and will make this harder to fix.
How about just make this an option to logging.setup() (could be notebook=... or grid=... depending on how you want to frame the question). Then the logger can be set up in both ways depending on usage?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1951. The problem where logging messages were shown in the incorrect cell (the cell where
pint.logging.setup()was called) seems to have been caused by passing theenqueue=Trueoption to Loguru'slogger.add()function. This PR simply removes the option. I've confirmed that this works both in a newer environment that showed the issue, and in an older one, which didn't have this issue, even withenqueue=True.I'm not sure why we were using
enqueue=Trueto begin with. The docstring forlogger.add()says this about it:Hopefully getting rid of this option (and switching back to
enqueue=False, which is the default in Loguru) will not cause issues in some use case I'm not aware of involving multiple PINT processes trying to log to a single file. But I think most likely this option was originally added in an attempt to get logging in a Jupyter notebook to work better, which is now backfiring.