Single submit file #112
Single submit file #112
Conversation
|
Thank you for the PR @ChromeHearts! I'm currently at a conference, but will review this PR when I get back. If I don't review after a week, please feel free to bug me : ) |
ChromeHearts
force-pushed
the
single_submit_file
branch
from
1490083
to
6d802ca
Compare
|
@jrbourbeau I have rebased my PR with the latest master and added unites for the single_submit_file dag. Please help review and merge it. I made sure it passed all unittests as well. Thanks! |
|
Thank you for the update @ChromeHearts! Apologies for letting this PR linger. This is a valuable contribution Just wanted to let you know that I'm fairly busy this week, but will set aside time later in the week to review this PR. Again, thanks for your work here and your patience |
There was a problem hiding this comment.
Overall this looks good @ChromeHearts!
I've left a few comments, some minor and some that will require a little more work. Not sure how much time you have to work on this — I'm happy to help out where needed
| # Add retry line for Job | ||
| if retry is not None: | ||
| retry_line = 'Retry {} {}'.format(node_name, retry) | ||
| job_arg_lines.append(retry_line) | ||
|
|
||
| return job_arg_lines | ||
|
|
||
| def build(self, makedirs=True, fancyname=True): | ||
| def build(self, makedirs=True, fancyname=True, single_file=False): | ||
| """Build and saves the submit file for Dagman |
There was a problem hiding this comment.
The single_file parameter should be added to the build() method docstring
| else: | ||
| name = node._get_fancyname() if fancyname else node.name | ||
| node.submit_name = name | ||
| node._has_arg_names = any([arg.name for arg in node.args]) | ||
| elif isinstance(node, Dagman): | ||
| node.build(makedirs, fancyname) |
There was a problem hiding this comment.
single_file should be passed to sub-Dagman build() calls
| node.build(makedirs, fancyname) | |
| node.build(makedirs=makedirs, fancyname=fancyname, single_file=single_file) |
| @@ -9,6 +9,21 @@ | |||
| from .visualize import visualize as _visualize | |||
|
|
|||
|
|
|||
| VAR_ATTR = ['executable', | |||
There was a problem hiding this comment.
Currently, when single_file=True the Job submit file extra_lines aren't included. Including it may be as simple as adding extra_lines to VAR_ATTR and adding $(extra_lines) to the Job submit file.
|
|
||
| if not os.path.exists(job_submit_file): | ||
| # Write lines to the single job submit file | ||
| with open(job_submit_file, 'w') as file: |
There was a problem hiding this comment.
This is a minor comment, but can you replace file here with something else, perhaps f or submit_file, to avoid confusion with the built-in file function in Python 2
| prior to using _iter_job_args. | ||
| prior to using _iter_job_args in non-single_file mode | ||
|
|
||
| single_file : single_file |
There was a problem hiding this comment.
See my other comment about the single_file description
| @@ -404,12 +468,15 @@ def build_submit(self, makedirs=True, fancyname=True, submit_options=None): | |||
| <http://research.cs.wisc.edu/htcondor/manual/current/condor_submit_dag.html>`_ | |||
| for possible options). | |||
|
|
|||
| single_file: bool, optional | |||
| When it's true, single job submission file will be generated. Default is False. | |||
There was a problem hiding this comment.
Minor comment. Can you update the single_file description to describe what happens when single_file=True and when single_file=False. Perhaps something like:
| When it's true, single job submission file will be generated. Default is False. | |
| Option to create a single ``Job`` submit file. If ``True``, a single ``Job`` submit file will be created and variables for each ``Job`` will be defined in the corresponding ``Dagman`` submit file. If ``False``, then a submit file is generated for each ``Job`` being managed by a ``Dagman`` (default is ``False``). |
| @@ -293,3 +296,29 @@ def test_dagman_add_node_ignores_duplicates(tmpdir, dagman): | |||
| dagman.add_job(job) | |||
|
|
|||
| assert dagman.nodes == [job] | |||
|
|
|||
|
|
|||
| def test_dagman_build_single_file(tmpdir): | |||
There was a problem hiding this comment.
This test should be expanded to ensure that all Job submit file attributes (e.g. queue, log, etc.) are included when single_file=True. Using the pytest.mark.parametrize decorator could be useful here.
|
Thanks for the review! I will look into them in a few days! |
Reference Issue
#95
What does this pull request implement/fix? Explain your changes.
Added single_file in dagman.build() to build submit files in single_submit_file mode.
Any other comments?
I have rebased with the latest master and made sure all unittests passed