-
Notifications
You must be signed in to change notification settings - Fork 203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Customize shebang 739 #940
Changes from 19 commits
004c3b2
8178cdb
3799315
ccdc450
d9a5bf5
ed612c1
4a27bce
b224ee7
5050ccc
b2001da
e308ad0
7206e89
6fc7daa
0390233
f112369
8c7c9b9
8ca5c64
ff96a5c
af7e3b6
68758fc
de8e5ef
6b08696
49633da
1278e4d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ localhost | |
True | ||
ssh | ||
torque | ||
#!/bin/bash | ||
/scratch/{username}/aiida_run | ||
mpirun -np {tot_num_mpiprocs} | ||
1 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -111,6 +111,7 @@ def full_text_info(self): | |
ret_lines.append(" * Transport type: {}".format(self.get_transport_type())) | ||
ret_lines.append(" * Scheduler type: {}".format(self.get_scheduler_type())) | ||
ret_lines.append(" * Work directory: {}".format(self.get_workdir())) | ||
ret_lines.append(" * Shebang: {}".format(self.get_shebang())) | ||
ret_lines.append(" * mpirun command: {}".format(" ".join( | ||
self.get_mpirun_command()))) | ||
def_cpus_machine = self.get_default_mpiprocs_per_machine() | ||
|
@@ -232,8 +233,13 @@ def set_workdir(self, val): | |
metadata = self._get_metadata() | ||
metadata['workdir'] = val | ||
self._set_metadata(metadata) | ||
#else: | ||
# raise ModificationNotAllowed("Cannot set a property after having stored the entry") | ||
|
||
def get_shebang(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't this logic be put in the abstract class? It's a pity that it's copy-pasted, and also even more dangerously the default is duplicated. I think the function can just stay in the base class instead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't think it should. The fact that there is a member called dbcomputer is a choice of the implementation. Some other backend could call it _dbcomputer or whatever, it's not enforced. As well as the function get_shebang, also not enforced. I would leave it where it is. |
||
try: | ||
return self.dbcomputer.get_shebang() | ||
except ConfigurationError: | ||
# This happens the first time: I provide a reasonable default value | ||
return "#!/bin/bash" | ||
|
||
def get_name(self): | ||
return self.dbcomputer.name | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -908,6 +908,7 @@ def test_submit_script(self): | |
s = PbsproScheduler() | ||
|
||
job_tmpl = JobTemplate() | ||
job_tmpl.shebang = '#!/bin/bash -l' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You put a |
||
job_tmpl.job_resource = s.create_job_resource(num_machines=1, num_mpiprocs_per_machine=1) | ||
job_tmpl.uuid = str(uuid.uuid4()) | ||
job_tmpl.max_wallclock_seconds = 24 * 3600 | ||
|
@@ -920,11 +921,39 @@ def test_submit_script(self): | |
submit_script_text = s.get_submit_script(job_tmpl) | ||
|
||
self.assertTrue('#PBS -r n' in submit_script_text) | ||
self.assertTrue(submit_script_text.startswith('#!/bin/bash')) | ||
self.assertTrue(submit_script_text.startswith('#!/bin/bash -l')) | ||
self.assertTrue('#PBS -l walltime=24:00:00' in submit_script_text) | ||
self.assertTrue('#PBS -l select=1' in submit_script_text) | ||
self.assertTrue("'mpirun' '-np' '23' 'pw.x' '-npool' '1'" + \ | ||
" < 'aiida.in'" in submit_script_text) | ||
def test_submit_script_bad_shebang(self): | ||
""" | ||
Test to verify if scripts works fine with default options | ||
""" | ||
from aiida.scheduler.datastructures import JobTemplate | ||
from aiida.common.datastructures import CodeInfo, code_run_modes | ||
|
||
s = PbsproScheduler() | ||
code_info = CodeInfo() | ||
code_info.cmdline_params = ["mpirun", "-np", "23", "pw.x", "-npool", "1"] | ||
code_info.stdin_name = 'aiida.in' | ||
|
||
for (shebang, expected_first_line) in ((None, '#!/bin/bash'), ("",""), ("NOSET", '#!/bin/bash')): | ||
job_tmpl = JobTemplate() | ||
if shebang == "NOSET": | ||
pass | ||
else: | ||
job_tmpl.shebang = shebang | ||
job_tmpl.job_resource = s.create_job_resource(num_machines=1, num_mpiprocs_per_machine=1) | ||
job_tmpl.codes_info = [code_info] | ||
job_tmpl.codes_run_mode = code_run_modes.SERIAL | ||
|
||
submit_script_text = s.get_submit_script(job_tmpl) | ||
|
||
# This tests if the implementation correctly chooses the default: | ||
self.assertEquals(submit_script_text.split('\n')[0], expected_first_line) | ||
|
||
|
||
|
||
def test_submit_script_with_num_cores_per_machine(self): | ||
""" | ||
|
@@ -937,6 +966,7 @@ def test_submit_script_with_num_cores_per_machine(self): | |
s = PbsproScheduler() | ||
|
||
job_tmpl = JobTemplate() | ||
job_tmpl.shebang = '#!/bin/bash' | ||
job_tmpl.job_resource = s.create_job_resource( | ||
num_machines=1, | ||
num_mpiprocs_per_machine=2, | ||
|
@@ -973,6 +1003,7 @@ def test_submit_script_with_num_cores_per_mpiproc(self): | |
s = PbsproScheduler() | ||
|
||
job_tmpl = JobTemplate() | ||
job_tmpl.shebang = '#!/bin/bash' | ||
job_tmpl.job_resource = s.create_job_resource( | ||
num_machines=1, | ||
num_mpiprocs_per_machine=1, | ||
|
@@ -1013,6 +1044,7 @@ def test_submit_script_with_num_cores_per_machine_and_mpiproc1(self): | |
s = PbsproScheduler() | ||
|
||
job_tmpl = JobTemplate() | ||
job_tmpl.shebang = '#!/bin/bash' | ||
job_tmpl.job_resource = s.create_job_resource( | ||
num_machines=1, | ||
num_mpiprocs_per_machine=1, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this was intentional irony, but you actually swapped the
#
and!
around in the conditional 😄There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if this is fixed, we still need @sphuber to accept the changes before merging?