[bugfix] Filter out empty strings before writing preamble #410
Conversation
Codecov Report
@@ Coverage Diff @@
## master #410 +/- ##
=========================================
+ Coverage 91.2% 91.2% +<.01%
=========================================
Files 70 70
Lines 8582 8586 +4
=========================================
+ Hits 7827 7831 +4
Misses 755 755
Continue to review full report at Codecov.
|
|
|
||
|
|
||
| _RFM_TRAP_ERROR = ''' | ||
| _RFM_TRAP_ERROR = r''' |
There was a problem hiding this comment.
Question: Which python version are you running and you're getting the string warnings?
There was a problem hiding this comment.
Pycodestyle shows me the warnings regarding invalid escape sequences. You must run with warnings enabled in order to take then from Python.
reframe/core/shell.py
Outdated
| def finalize(self): | ||
| ret = '\n'.join([self.shebang, *self._prolog, | ||
| *self._body, *self._epilog]) | ||
| # Here empty strings are filtered out before joining |
There was a problem hiding this comment.
Write it as "Filter out empty statements before returning". And return directly, without using the stripped_shell variable.
There was a problem hiding this comment.
Nonetheless, have you found the place that actually generated these empty instructions? Perhaps, we could fix the problem there.
There was a problem hiding this comment.
I would also expand the shell script generator tests to check this stripping.
There was a problem hiding this comment.
Yes, the problem begins from _format_option of reframe/core/schedulers/slurm.py. If the regression test does not set some fields whose default values are None the _format_option will return an empty string at the corresponding line which will be appended to the list of lines passed to ShellScriptGenerator. Thus, when finalizing, during joining with \n all these empty strings will be replaced by empty lines.
There was a problem hiding this comment.
As discussed offline, I think it's better to fix that in the Slurm job script generator.
* Use `filter` to filter out empty strings before finalizing shell. * User raw string to suppress invalid escape sequence warning.
* Perform the filtering of empty strings before emitting the slurm preamble. * Create unittest to test preamble for empty lines.
teojgo
force-pushed
the
bugfix/empty_lines_shell
branch
from
2fc1fef to
bc2fe46
Compare
teojgo
changed the title
reframe/core/schedulers/slurm.py
Outdated
| preamble.append(opt) | ||
|
|
||
| return preamble | ||
| # Filter out empty statements before returning" |
There was a problem hiding this comment.
Remove the double quotes at the end.
unittests/test_schedulers.py
Outdated
| with open(self.testjob.script_filename) as fp: | ||
| self.assertIsNotNone(re.search(r'--hint=nomultithread', fp.read())) | ||
|
|
||
| def test_no_empty_lines_in_preamble(self): |
There was a problem hiding this comment.
This test is not specific to the Slurm backend. Instead it must be part of the base _TestJob. I am also not sure that you have to set the different attributes to None explicitly, since the default setUp() method sets up a minimal job.
| self.testjob.prepare(self.commands, self.environs) | ||
| self.assertRaises(JobNotStartedError, self.testjob.finished) | ||
|
|
||
| def test_no_empty_lines_in_preamble(self): |
There was a problem hiding this comment.
This should be part of the _TestJob base class. Subclasses need not re-implement this, since it should be the same everywhere.
unittests/test_schedulers.py
Outdated
| self.assertRaises(JobNotStartedError, self.testjob.finished) | ||
|
|
||
| def test_no_empty_lines_in_preamble(self): | ||
| self.setup_job() |
There was a problem hiding this comment.
You should not call setup_job() here, because this sets everything and most probably the following test will have no meaning. I suggest omitting completely this call. In this case, self.testjob will be minimal.
|
@jenkins-cscs retry all |
|
@jenkins-cscs retry dom |
Use
filterto filter out empty strings before finalizing shell.User raw string to suppress invalid escape sequence warning.
Fixes #407