Skip to content
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

Merged
merged 24 commits into from
Dec 21, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
004c3b2
First work on #739, in which I make it possible to store shebang in t…
lekah Nov 17, 2017
8178cdb
Updated tests that were failing for #739
lekah Nov 17, 2017
3799315
for #739 the JobTemplate instance now gets the shebang line from the …
lekah Nov 17, 2017
ccdc450
Updated the travis-test for the daemon for #739
lekah Nov 17, 2017
d9a5bf5
Updated scheduler test (#739)
lekah Nov 17, 2017
ed612c1
Merge branch 'develop' into customize_shebang_739
lekah Nov 20, 2017
4a27bce
Minor fix for Configuration Error message in sqlalchemy's DbComputer
lekah Nov 21, 2017
b224ee7
Merge branch 'develop' into customize_shebang_739
lekah Nov 21, 2017
5050ccc
Merge branch 'develop' into customize_shebang_739
lekah Nov 25, 2017
b2001da
Merge branch 'develop' into customize_shebang_739
lekah Nov 29, 2017
e308ad0
Merge branch 'develop' into customize_shebang_739
lekah Dec 15, 2017
7206e89
Merge branch 'customize_shebang_739' of github.com:lekah/aiida_core i…
lekah Dec 15, 2017
6fc7daa
For #739 moved the set_shebang into the abstract class
lekah Dec 15, 2017
0390233
For #739 I check the jobTemplate's shebang attribute, in order to pro…
lekah Dec 15, 2017
f112369
For #739 I test the default shebang line
lekah Dec 15, 2017
8c7c9b9
Added documentation for #739
lekah Dec 15, 2017
8ca5c64
Changed the order of the computer setup so that shebang line is asked…
lekah Dec 15, 2017
ff96a5c
Added new check if user specified an empty string for the shebang lin…
lekah Dec 15, 2017
af7e3b6
Improved exception message when setting shebabgn
lekah Dec 15, 2017
68758fc
Corrected small bag in check for shebang line
lekah Dec 15, 2017
de8e5ef
Still related to changing the order of shebang and workdir
lekah Dec 15, 2017
6b08696
Fixed docstring
lekah Dec 15, 2017
49633da
Merge branch 'develop' into customize_shebang_739
lekah Dec 18, 2017
1278e4d
Merge branch 'develop' into customize_shebang_739
giovannipizzi Dec 20, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .travis-data/computer-setup-input.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ localhost
True
ssh
torque
#!/bin/bash
/scratch/{username}/aiida_run
mpirun -np {tot_num_mpiprocs}
1
Expand Down
21 changes: 14 additions & 7 deletions aiida/backends/djsite/db/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -1426,21 +1426,28 @@ def get_aiida_class(self):
from aiida.orm.computer import Computer
return Computer(dbcomputer=self)

def get_workdir(self):

def _get_val_from_metadata(self, key):
import json

try:
metadata = json.loads(self.metadata)
except ValueError:
raise DbContentError(
"Error while reading metadata for DbComputer {} ({})".format(
self.name, self.hostname))

"Error while reading metadata for DbComputer {} ({})".format(self.name, self.hostname))
try:
return metadata['workdir']
return metadata[key]
except KeyError:
raise ConfigurationError('No workdir found for DbComputer {} '.format(
self.name))
raise ConfigurationError('No {} found for DbComputer {} '.format(key,self.name))

def get_workdir(self):
return self._get_val_from_metadata('workdir')

def get_shebang(self):
"""
Return the shebang line
"""
return self._get_val_from_metadata('shebang')

def __str__(self):
if self.enabled:
Expand Down
7 changes: 7 additions & 0 deletions aiida/backends/sqlalchemy/models/computer.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,13 @@ def get_workdir(self):
raise ConfigurationError('No workdir found for DbComputer {} '.format(
self.name))

def get_shebang(self):
try:
return self._metadata['shebang']
except KeyError:
raise ConfigurationError('No shebang found for DbComputer {} '.format(
self.name))

@property
def pk(self):
return self.id
Expand Down
1 change: 1 addition & 0 deletions aiida/backends/tests/verdi_commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
"True",
"ssh",
"torque",
"#!/bin/bash",
"/scratch/{username}/aiida_run",
"mpirun -np {tot_num_mpiprocs}",
"1",
Expand Down
12 changes: 10 additions & 2 deletions aiida/orm/implementation/django/computer.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ def full_text_info(self):
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()
Expand Down Expand Up @@ -220,6 +221,14 @@ def get_workdir(self):
# This happens the first time: I provide a reasonable default value
return "/scratch/{username}/aiida_run/"

def get_shebang(self):
try:
return self.dbcomputer.get_shebang()
except ConfigurationError:
# This happens the first time: I provide a reasonable default value
return "#!/bin/bash"


def set_workdir(self, val):
# if self.to_be_stored:
if not isinstance(val, basestring):
Expand All @@ -228,8 +237,7 @@ 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_name(self):
return self.dbcomputer.name
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1453,6 +1453,7 @@ def _presubmit(self, folder, use_unstored_links=False):

# I create the job template to pass to the scheduler
job_tmpl = JobTemplate()
job_tmpl.shebang = computer.get_shebang()
## TODO: in the future, allow to customize the following variables
job_tmpl.submit_as_hold = False
job_tmpl.rerunnable = False
Expand Down
35 changes: 34 additions & 1 deletion aiida/orm/implementation/general/computer.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,11 @@ def _conf_attributes(self):
",".join(Scheduler.get_valid_schedulers())),
False,
),
("shebang",
"shebang line at the beginning of the submission script",
"this line specifies the first line of the submission script for this computer",
False,
),
("workdir",
"AiiDA work directory",
"The absolute path of the directory on the computer where AiiDA will\n"
Expand Down Expand Up @@ -449,6 +454,18 @@ def _set_workdir_string(self, string):
self._workdir_validator(string)
self.set_workdir(string)


def _get_shebang_string(self):
return self.get_shebang()

def _set_shebang_string(self, string):
"""
Set the shebang line.
"""
# self._shebang_validator(string)
# Should we validate?
self.set_shebang(string)

@classmethod
def _workdir_validator(cls, workdir):
"""
Expand Down Expand Up @@ -682,10 +699,26 @@ def set_transport_params(self, val):
def get_workdir(self):
pass

@abstractmethod
def get_shebang(self):
pass

@abstractmethod
def set_workdir(self, val):
pass

def set_shebang(self, val):
"""
:param str val: A valid shebang line
"""
if not isinstance(val, basestring):
raise ValueError("{} is invalid. Input has to be a string".format(val))
if not val.startswith('#!'):
raise ValueError("{} is invalid. A shebang line has to start with #!".format(val))
metadata = self._get_metadata()
metadata['shebang'] = val
self._set_metadata(metadata)

@abstractmethod
def get_name(self):
pass
Expand Down Expand Up @@ -809,4 +842,4 @@ class Util(object):

@abstractmethod
def delete_computer(self, pk):
pass
pass
10 changes: 8 additions & 2 deletions aiida/orm/implementation/sqlalchemy/computer.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Down
15 changes: 11 additions & 4 deletions aiida/scheduler/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ def get_submit_script(self, job_tmpl):

The plugin returns something like

#!/bin/bash <- this shebang line could be configurable in the future
#!/bin/bash <- this shebang line is configurable to some extent
scheduler_dependent stuff to choose numnodes, numcores, walltime, ...
prepend_computer [also from calcinfo, joined with the following?]
prepend_code [from calcinfo]
Expand All @@ -145,13 +145,20 @@ def get_submit_script(self, job_tmpl):

empty_line = ""

shebang = "#!/bin/bash"

# I fill the list with the lines, and finally join them and return
script_lines = []
script_lines.append(shebang)
script_lines.append(empty_line)

if job_tmpl.shebang:
script_lines.append(job_tmpl.shebang)
elif job_tmpl.shebang == '':
# Here I check whether the shebang was set explicitly as an empty line.
# In such a case, the first line is empty, if that's what the user wants:
script_lines.append(job_tmpl.shebang)
elif job_tmpl.shebang is None:
script_lines.append('#!/bin/bash')
else:
raise ValueError("Invalid shebang set: {}".format(job_tmpl.shebang))
script_lines.append(self._get_submit_script_header(job_tmpl))
script_lines.append(empty_line)

Expand Down
2 changes: 2 additions & 0 deletions aiida/scheduler/datastructures.py
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,7 @@ class JobTemplate(DefaultFieldsAttributeDict):

Fields:

* ``shebang line``: The first line of the submission script
* ``submit_as_hold``: if set, the job will be in a 'hold' status right
after the submission
* ``rerunnable``: if the job is rerunnable (boolean)
Expand Down Expand Up @@ -377,6 +378,7 @@ class JobTemplate(DefaultFieldsAttributeDict):
# place then.

_default_fields = (
'shebang',
'submit_as_hold',
'rerunnable',
'job_environment',
Expand Down
2 changes: 2 additions & 0 deletions aiida/scheduler/plugins/test_lsf.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ def test_submit_script(self):
s = LsfScheduler()

job_tmpl = JobTemplate()
job_tmpl.shebang = '#!/bin/bash'
job_tmpl.uuid = str(uuid.uuid4())
job_tmpl.job_resource = s.create_job_resource(tot_num_mpiprocs=2,
parallel_env='b681e480bd.cern.ch')
Expand All @@ -154,6 +155,7 @@ def test_submit_script(self):

submit_script_text = s.get_submit_script(job_tmpl)


self.assertTrue( submit_script_text.startswith('#!/bin/bash') )

self.assertTrue( '#BSUB -rn' in submit_script_text )
Expand Down
34 changes: 33 additions & 1 deletion aiida/scheduler/plugins/test_pbspro.py
Original file line number Diff line number Diff line change
Expand Up @@ -908,6 +908,7 @@ def test_submit_script(self):
s = PbsproScheduler()

job_tmpl = JobTemplate()
job_tmpl.shebang = '#!/bin/bash -l'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You put a -l as a way to verify the current implementation?
Maybe if you implement also the case with shebang not specified, you can have a copy of one of these tests to verify that even if you don't set it, you get the default value?

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
Expand All @@ -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):
"""
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
29 changes: 29 additions & 0 deletions aiida/scheduler/plugins/test_slurm.py
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ def test_submit_script(self):
s = SlurmScheduler()

job_tmpl = JobTemplate()
job_tmpl.shebang = '#!/bin/bash'
job_tmpl.uuid = str(uuid.uuid4())
job_tmpl.job_resource = s.create_job_resource(num_machines=1, num_mpiprocs_per_machine=1)
job_tmpl.max_wallclock_seconds = 24 * 3600
Expand All @@ -197,6 +198,31 @@ def test_submit_script(self):
self.assertTrue( "'mpirun' '-np' '23' 'pw.x' '-npool' '1'" + \
" < 'aiida.in'" in submit_script_text )

def test_submit_script_bad_shebang(self):
from aiida.scheduler.datastructures import JobTemplate
from aiida.common.datastructures import CodeInfo, code_run_modes

s = SlurmScheduler()
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):
"""
Test to verify if script works fine if we specify only
Expand All @@ -208,6 +234,7 @@ def test_submit_script_with_num_cores_per_machine(self):
s = SlurmScheduler()

job_tmpl = JobTemplate()
job_tmpl.shebang = '#!/bin/bash'
job_tmpl.job_resource = s.create_job_resource(
num_machines=1,
num_mpiprocs_per_machine=2,
Expand Down Expand Up @@ -244,6 +271,7 @@ def test_submit_script_with_num_cores_per_mpiproc(self):
s = SlurmScheduler()

job_tmpl = JobTemplate()
job_tmpl.shebang = '#!/bin/bash'
job_tmpl.job_resource = s.create_job_resource(
num_machines=1,
num_mpiprocs_per_machine=1,
Expand Down Expand Up @@ -284,6 +312,7 @@ def test_submit_script_with_num_cores_per_machine_and_mpiproc1(self):
s = SlurmScheduler()

job_tmpl = JobTemplate()
job_tmpl.shebang = '#!/bin/bash'
job_tmpl.job_resource = s.create_job_resource(
num_machines=1,
num_mpiprocs_per_machine=1,
Expand Down
Loading