Fix spinner-tqdm output conflict and refactor stream reading#45
Fix spinner-tqdm output conflict and refactor stream reading#45
Conversation
f541eca to
c7f6dd6
Compare
…ading - Spinner no longer overlaps with tqdm due to proper threading.Event handling - Score-P stdout and stderr are read in separate threads with chunked reading - Animation is disabled in multicell mode when needed - README.md and error logging improved
c7f6dd6 to
3e4f44d
Compare
src/scorep_jupyter/kernel.py
Outdated
| decoded_line = line.decode( | ||
| sys.getdefaultencoding(), errors="ignore" | ||
| ) | ||
| multicellmode_timestamps = self.read_scorep_stdout(proc.stdout, stdout_lock, spinner_stop_event) |
There was a problem hiding this comment.
what exactly is the purpose of the multicellmode_timestamps in the current version? In the past, we used them to visualize the different cells in the diagrams for coarse-grained measurements, e.g. CPU utilization. Since this was outsourced to ipython extension, I am not sure if we need the multicellmode_timestamps in the current version. I guess, you are disabling the spinner in case multiple cells should be tracked by Score-P (multicellmode) but perhaps, we do not need the timestamps anymore but just a flag for that case, right?
There was a problem hiding this comment.
Yes, We do not need multicellmode_timestamps there anymore. I removed them in the last commit #45. Regarding a flag, you are right, it is still necessary to suppress spinner animation in case of %%finalize_multicellmode command execution
| if is_enabled: | ||
| return BusySpinner(lock) | ||
| def create_busy_spinner(lock=None, stop_event=None, is_multicell_final=False): | ||
| is_enabled = os.getenv("SCOREP_JUPYTER_DISABLE_PROCESSING_ANIMATIONS") != "1" |
There was a problem hiding this comment.
Can we have a central definition for "True" values that we can use everywhere for checking the env variables?
e.g.
common.py
TRUTHY_VALS=['true', '1', 't']
somewhere in the code:
is_enabled = os.getenv("SCOREP_JUPYTER_DISABLE_PROCESSING_ANIMATIONS").lower() in TRUTHY_VALS
There was a problem hiding this comment.
For this I'd suggest implementing something similar to Django-style settings.py. Since we already have a number of environment variables (e.g., flags, paths to results, SCOREP_ variables, etc.) and are likely to introduce many more in the future, it makes sense to define a central configuration file - not just for flags, but for all project settings.
The benefits of this approach:
- All settings can be shared across all parts of application (e.g. kernel, extensions)
- Environment variables and other settings are defined and parsed only once - in a single place:
settings.py. - No more repetitive
os.getenv(SOME_VAR)calls scattered throughout the codebase - justfrom conf import settingsandsettings.SOME_VARto access any variable. - (Optional) Extend
settings.pyby logging config so we would have full configuration and control on our project in one place.
Finally (if needed) it can be extended to:
conf/
└── settings/
├── init.py - to choose between environments (prod - chosen by default with env var)
├── test.py - for unit tests (animations suppressed, SCOREP_* configured for tests, logging.DEBUG = True)
├── dev.py - e.g. logging.DEBUG = True by default
└── prod.py - e.g. logging.DEBUG = False
a180104 to
e1fa1b5
Compare
e1fa1b5 to
00e237a
Compare
Summary
This PR addresses issues with animation display when using
tqdmin conjunction with PyTorch and Score-P logging.Problems Fixed
Spinner animation worked with the PyTorch progress bar (tested in
examples/gpt-demo/demonstrator.ipynb), but was broken with the defaulttqdm.When
scorep_jupyter_DISABLE_PROCESSING_ANIMATIONS=1is set, spinner output overlapped withtqdm.Changes Made
Refactored reading of Score-P process streams: both
stdoutandstderrare now read in chunks using separate threads.Stream reader threads now wait on a shared
threading.Eventthat signals the termination of theprocess_busy_spinneranimation whenscorep_jupyter_DISABLE_PROCESSING_ANIMATIONS=0.Added a
is_multicell_final=Trueflag as ascorep_execute()function argument to suppress spinner creation during%%finalize_multicellmodewhenscorep_jupyter_DISABLE_PROCESSING_ANIMATIONS=0. This ensurescreate_busy_spinner()produces a dummy spinner without capturing the event.Notes
Fix ensures
tqdmworks with both PyTorch and default modes.When
scorep_jupyter_DISABLE_PROCESSING_ANIMATIONS=0is set (animation is enabled) - no Score-P child processstderroutput in console during animation. In other words, if user's code in cell raises an exception - no detailed error message is seen but only error log is captured. This behavior is described inREADME.md