Fix slurm.py to work correctly with dvsim#234
Conversation
sha-ron
force-pushed
the
dvsim_slurm
branch
2 times, most recently
from
408d41c to
e462459
Compare
AlexJones0
left a comment
AlexJones0
left a comment
There was a problem hiding this comment.
Thanks! I can't test this locally at the moment but the changes seem like reasonable fixes to me. Just one comment that I'm not entirely sure about and then I think this is good to go in.
| self.process = subprocess.Popen( | ||
| shlex.split(slurm_cmd), | ||
| bufsize=4096, | ||
| universal_newlines=True, |
There was a problem hiding this comment.
Perhaps I'm missing something - do we not still need universal_newlines=True (or text=True) since we're trying to write to a log file opened in w mode and not wb?
There was a problem hiding this comment.
Thank you for the review @AlexJones0.
Slurm works after applying the changes in this PR (at least for me). So I guess anyway it is better then what we have now that fails on compilation.
As for the note: honestly, AI claimed it is not needed (see explanation below) and indeed it worked fine without it. But I added it again just in case.
"When Popen redirects stdout/stderr to a file object, it uses dup2 on the underlying file descriptor — the child process writes directly to the fd, bypassing the Python file object entirely. So text vs binary mode on the Python side doesn't matter".
There was a problem hiding this comment.
That makes sense, thanks. I think you are right that in this case it doesn't matter, albeit the subprocess docs aren't the most clear about this detail 😅
Signed-off-by: Sharon Topaz <sharon.topaz@nuvoton.com> fix(dvsim): fix lint issues Signed-off-by: Sharon Topaz <sharon.topaz@nuvoton.com> fix(dvsim): fix PR note Signed-off-by: Sharon Topaz <sharon.topaz@nuvoton.com>
2 participants
Fixed
unexpected end of filesyntax error.A few more issues that were fixed:
• dvsim hangs indefinitely after job completes
• No pass/fail summary displayed
• Timer/progress display frozen