Added support for another job scheduler. #24642
Conversation
venkatk-ot
force-pushed
the
master
branch
2 times, most recently
from
8348c09
to
fd1f3f0
Compare
venkatk-ot
force-pushed
the
master
branch
6 times, most recently
from
077684e
to
6325edb
Compare
util/dvsim/NcLauncher.py
Outdated
| self.process = subprocess.Popen(cmd_arr, | ||
| stdin=None, | ||
| stdout=None, | ||
| stderr=subprocess.STDOUT, | ||
| # string mode | ||
| universal_newlines=True, | ||
| env=exports, | ||
| cwd=self.deploy.odir) | ||
|
|
||
| # Wait until the process exit | ||
| self.process.wait() |
There was a problem hiding this comment.
For the blocking version that we're doing here, do we really need Popen? Isn't it equivalent to do this?
subprocess.run(cmd_arr,
stdin=None,
stdout=None,
stderr=subprocess.STDOUT,
universal_newlines=True,
env=exports,
cwd=self.deploy.odir)That way, we don't need to talk about self.process at all for interactive mode.
There was a problem hiding this comment.
I tried this, but it did not work. The reason is the poll function seems to get called even for interactive job and it has code that uses self.process variable. can i leave this as it is or should i rework the poll function for interactive job ?
There was a problem hiding this comment.
Hmm, the code is a bit ugly here. I think the call to poll comes from Scheduler._poll? Maybe the best approach is to set self.status after the run call has finished? Then NcLauncher.poll can do the same thing as LsfLauncher.poll and return it immediately.
|
Thanks for the tweaks you have made since my first review. I realise that there was quite a long list of items in that review. I think the only ones open are:
|
venkatk-ot
force-pushed
the
master
branch
4 times, most recently
from
26c3ef9
to
f66b7a6
Compare
util/dvsim/NcLauncher.py
Outdated
| encoding='UTF-8', | ||
| errors='surrogateescape') as f: | ||
| f.write('[Executing]:\n{}\n\n'.format(self.deploy.cmd)) | ||
| self.process = subprocess.Popen(cmd_arr, |
There was a problem hiding this comment.
Oof: this looks a little surprising to me (which I think might have been my fault!) Does't Popen exit almost immediately? The with block will then finish. Presumably Python avoids closing the fds because they are pointed to by self.process.
My suggestion was terrible! Probably best to keep the explicit open call that you had before (possibly with a comment to avoid confusing the next person like me...). I'm really sorry for the noise.
There was a problem hiding this comment.
done . reverted the changes with a comment explaining the reason
util/dvsim/NcLauncher.py
Outdated
| f = open(self.deploy.get_log_path(), | ||
| 'w', | ||
| encoding='UTF-8', | ||
| errors='surrogateescape') | ||
| f.write('[Executing]:\n{}\n\n'.format(self.deploy.cmd)) | ||
| f.flush() |
There was a problem hiding this comment.
Thanks. Looking more carefully, I think my suggestion was a bit silly. I'll leave a proper note on the newly changed lines as well, but I'm really sorry for confusing stuf
venkatk-ot
force-pushed
the
master
branch
2 times, most recently
from
4402d7c
to
d8e1962
Compare
venkatk-ot
requested review from
mundaym,
a team,
vogelpi and
msfschaffner
as code owners
venkatk-ot
requested review from
marnovandermaas and
HU90m
and removed request for
a team
|
reopening after resync |
|
reopen after resync |
|
Hi @rswarbrick i tried to sync my fork to master as there were 400+ commits . but in the process this PR got closed. can you please help me reopen this PR or should i create a new PR ? |
|
I tried it from my side, but the best I could do was to create a new PR here |
|
. |
|
I'm not sure exactly what happened here, but it's no problem: we'll jump over to that PR. Thanks for the links. |
NetworkComputer(NC) now also known as Altair accelerator is a commercial job scheduler tool from Altair/RunTime This change also includes post-processing script to filter out exit code 6, which is a false error code for jobs run in NC.