fix: tqdm logs within wrap_rank [MLG-1236] #8488
Conversation
✅ Deploy Preview for determined-ui canceled.
|
| line = f"[rank={rank}] ".encode() + line | ||
| os.write(dst_stream.fileno(), line) | ||
|
|
||
| # Duplicated in ship_logs.py. If you find a bug here, fix it there too. |
There was a problem hiding this comment.
This and its mate are nice comments. Not sure I'd have thought to do it.
| from determined.launch import wrap_rank | ||
|
|
||
|
|
||
| def test_split_on_new_lines_or_carriage_returns() -> None: |
There was a problem hiding this comment.
This is a little pedantic, but I like it when tests test one thing. Even if that means copy/pasted code. Here, what do you think of two tests; one for \n and one for \r?
There was a problem hiding this comment.
Possibly also one for stdin one for stderr? Maybe it's worth a short chat about what that division is testing.
There was a problem hiding this comment.
resolved; opted to remove the stderr writes. They were intended to be signals but since the stdout reads were blocking it actually made no difference.
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| ) | ||
| assert p.stdin and p.stdout and p.stderr |
There was a problem hiding this comment.
The more assertions, the less it's clear to me what's under test. Short chat?
There was a problem hiding this comment.
discussed offline, added a comment to the mypy assertion and agreed that the p.wait()==0 is just about being strict, not truly its own test.
rb-determined-ai
force-pushed
the
rb/tqdm
branch
from
61aa847
to
672c6a1
Compare
rb-determined-ai
force-pushed
the
rb/tqdm
branch
2 times, most recently
from
bb40054
to
1eb351c
Compare
docs/release-notes/8488-tqdm.rst
Outdated
| - The ``determined.launch.wrap_rank`` module, often used by custom launch layers, had a bug where | ||
| it was improperly buffering multiple lines separated by a carriage return, such as logs emitted | ||
| from the popular TQDM library. That has been fixed, so TQDM logs should pass through without | ||
| undue buffering. |
There was a problem hiding this comment.
:orphan:
Bug Fixes
- Fix an issue where the
determined.launch.wrap_rankmodule, often used by custom launch layers, was improperly buffering multiple lines separated by a carriage return, such as logs emitted
from the popular TQDM library. TQDM logs will now pass through without undue buffering.
This has already been fixed in ship_logs.py, but since ship_logs.py isn't allowed to to have any non-stdlib dependencies (including determined) and since determined can't depend on ship_logs.py (which is only available on-cluster), the right answer is copy/paste.
rb-determined-ai
force-pushed
the
rb/tqdm
branch
from
1eb351c
to
456adec
Compare
This has already been fixed in ship_logs.py, but since ship_logs.py isn't allowed to to have any non-stdlib dependencies (including determined) and since determined can't depend on ship_logs.py (which is only available on-cluster), the right answer is copy/paste.