Feature/scorep feedback #42
Merged
Conversation
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
…mprovements 1. Added and documented a logging system. 2. Implemented animation for long-running processes in Score-P mode (both for data transfer to the subprocess and for displaying progress animation within the subprocess itself). 3. Added an example of execution of the long running task – Large array processing with Score-P in `examples/ExampleBasic.ipynb`. 4. Changed the way subprocess output streams are read — moved this logic into a separate function `read_scorep_process_pipe()`. 5. Brought tests into a working state: - Created a new function to clean up garbage from standard output. - Added a context manager `with self.subTest()` to better identify which cell failed during execution. - Temporarily updated expected test outputs to account for a new line introduced for proper animation handling (`clear_line` string in `read_scorep_process_pipe()`).
OutlyingWest
force-pushed
the
feature/scorep_feedback
branch
from
03754b2 to
1e56026
Compare
elwer
requested changes
May 26, 2025
There was a problem hiding this comment.
For logging, I have the following recommendations:
- Check whether the logging levels are appropriate. For example, on line 923 of kernel.py, the message '{os.environ["SCOREP_EXPERIMENT_DIRECTORY"]=}' should not be logged as a warning.
- Check the consistency of the logging messages. For example, we have variations of the same message:
'Cell execution failed, cell persistence was not recorded', 'Failed to load cell's persistence to the notebook', 'Failed to pickle notebook's persistence'. These sound relatively similar, although there are differences. Perhaps you could make them more meaningful by adding the context of the persistence step (e.g. Jupyter parent -> Scorep child). You could also add the type of serialisation (memory/disk, serialiser, etc.).
What could help here is an enum to define the potential error messages and, for the future, hints on how to resolve them.
elwer
reviewed
May 26, 2025
src/jumper/kernel.py
Outdated
Comment on lines
969
to
1016
|
|
||
| def read_scorep_process_pipe(self, proc: subprocess.Popen[bytes], stdout_lock: threading.Lock) -> list: | ||
| """ | ||
| Reads and processes the output of a subprocess running with Score-P instrumentation. | ||
| Args: | ||
| proc (subprocess.Popen[bytes]): The subprocess whose output is being read. | ||
| stdout_lock (threading.Lock): Lock to avoid output overlapping | ||
|
|
||
| Returns: | ||
| list: A list of decoded strings containing "MCM_TS" timestamps. | ||
| """ | ||
| multicellmode_timestamps = [] | ||
| sel = selectors.DefaultSelector() | ||
|
|
||
| sel.register(proc.stdout, selectors.EVENT_READ) | ||
| sel.register(proc.stderr, selectors.EVENT_READ) | ||
|
|
||
| line_width = 50 | ||
| clear_line = "\r" + " " * line_width + "\r" | ||
|
|
||
| while True: | ||
| # Select between stdout and stderr | ||
| for key, val in sel.select(): | ||
| line = key.fileobj.readline() | ||
| if not line: | ||
| sel.unregister(key.fileobj) | ||
| continue | ||
|
|
||
| decoded_line = line.decode(sys.getdefaultencoding(), errors='ignore') | ||
|
|
||
| if key.fileobj is proc.stderr: | ||
| with stdout_lock: | ||
| self.log.warning(f'{decoded_line.strip()}') | ||
| elif 'MCM_TS' in decoded_line: | ||
| multicellmode_timestamps.append(decoded_line) | ||
| else: | ||
| with stdout_lock: | ||
| sys.stdout.write(clear_line) | ||
| sys.stdout.flush() | ||
| self.cell_output(decoded_line) | ||
|
|
||
| # If both stdout and stderr empty -> out of loop | ||
| if not sel.get_map(): | ||
| break | ||
|
|
||
| return multicellmode_timestamps | ||
|
|
||
|
|
There was a problem hiding this comment.
Please add a bit of documentation here
There was a problem hiding this comment.
Documentation extended in 26c9b81
elwer
reviewed
May 26, 2025
added 5 commits
May 27, 2025 14:01
…re fixed in branch 'test/improvements'
|
elwer
approved these changes
Jun 5, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of Changes
examples/ExampleBasic.ipynbread_scorep_process_pipe().with self.subTest()to better identify which cell failed during execution.clear_linestring inread_scorep_process_pipe()).