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

Implement default submission template #338

Merged
merged 21 commits into from
Sep 28, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
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
10 changes: 5 additions & 5 deletions pysqa/utils/basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from pysqa.utils.execute import execute_command
from pysqa.utils.queues import Queues
from pysqa.utils.validate import value_error_if_none, value_in_range
from pysqa.wrapper.generic import SchedulerCommands
from pysqa.wrapper.abstract import SchedulerCommands

queue_type_dict = {
"SGE": {
Expand Down Expand Up @@ -511,14 +511,14 @@ def _job_submission_template(
memory_max=memory_max,
active_queue=active_queue,
)
template = active_queue["template"]
return template.render(
job_name=job_name,
return self._commands.render_submission_template(
command=command,
submission_template=active_queue["template"],
working_directory=working_directory,
job_name=job_name,
cores=cores,
memory_max=memory_max,
run_time_max=run_time_max,
command=command,
dependency_list=dependency_list,
**kwargs,
)
Expand Down
64 changes: 54 additions & 10 deletions pysqa/wrapper/generic.py → pysqa/wrapper/abstract.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@
# Copyright (c) Max-Planck-Institut für Eisenforschung GmbH - Computational Materials Design (CM) Department
# Distributed under the terms of "New BSD License", see the LICENSE file.

import os
from abc import ABC, abstractmethod
from typing import List, Optional, Union

import pandas
from jinja2 import Template

__author__ = "Niklas Siemer"
__copyright__ = (
Expand All @@ -19,6 +22,16 @@


class SchedulerCommands(ABC):
@property
def enable_reservation_command(self) -> list[str]:
"""
Returns the command to enable job reservation on the scheduler.

Returns:
list[str]: The command to enable job reservation.
"""
raise NotImplementedError()

@property
@abstractmethod
def submit_job_command(self) -> list[str]:
Expand All @@ -41,16 +54,6 @@ def delete_job_command(self) -> list[str]:
"""
pass

@property
def enable_reservation_command(self) -> list[str]:
"""
Returns the command to enable job reservation on the scheduler.

Returns:
list[str]: The command to enable job reservation.
"""
raise NotImplementedError()

@property
@abstractmethod
def get_queue_status_command(self) -> list[str]:
Expand All @@ -62,6 +65,47 @@ def get_queue_status_command(self) -> list[str]:
"""
pass

@staticmethod
def render_submission_template(
command: str,
submission_template: Union[str, Template],
job_name: str = "pysqa",
working_directory: str = os.path.abspath("."),
cores: int = 1,
memory_max: Optional[int] = None,
run_time_max: Optional[int] = None,
dependency_list: Optional[List[int]] = None,
**kwargs,
) -> str:
"""
Generate the job submission template.

Args:
command (str, optional): The command to be executed.
job_name (str, optional): The job name. Defaults to "pysqa".
working_directory (str, optional): The working directory. Defaults to ".".
cores (int, optional): The number of cores. Defaults to 1.
memory_max (int, optional): The maximum memory. Defaults to None.
run_time_max (int, optional): The maximum run time. Defaults to None.
dependency_list (list[int], optional): The list of dependency job IDs. Defaults to None.
submission_template (str): Submission script template pysqa.wrapper.torque.template

Returns:
str: The rendered job submission template.
"""
if not isinstance(submission_template, Template):
submission_template = Template(submission_template)
return submission_template.render(
command=command,
job_name=job_name,
working_directory=working_directory,
cores=cores,
memory_max=memory_max,
run_time_max=run_time_max,
dependency_list=dependency_list,
**kwargs,
)

@staticmethod
def dependencies(dependency_list: list[str]) -> list:
"""
Expand Down
65 changes: 64 additions & 1 deletion pysqa/wrapper/flux.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,31 @@
# coding: utf-8
import os
from typing import List, Optional, Union

import pandas
from flux.job import JobID
from jinja2 import Template

from pysqa.wrapper.abstract import SchedulerCommands

from pysqa.wrapper.generic import SchedulerCommands
template = """\
#!/bin/bash
# flux: --job-name={{job_name}}
# flux: --env=CORES={{cores}}
# flux: --output=time.out
# flux: --error=error.out
Comment on lines +14 to +15
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Make output and error file names configurable

Currently, the output and error file names are hardcoded as time.out and error.out. To provide flexibility, consider making these file names configurable through function parameters.

Update the function signature to include new parameters:

 def render_submission_template(
     command: str,
     job_name: str = "pysqa",
     working_directory: str = os.path.abspath("."),
     cores: int = 1,
     memory_max: Optional[int] = None,
     run_time_max: Optional[int] = None,
     dependency_list: Optional[List[int]] = None,
+    output_file: str = "time.out",
+    error_file: str = "error.out",
     submission_template: Union[str, Template] = template,
     **kwargs,
 ) -> str:

Update the template to use these parameters:

 # flux: --output=time.out
 # flux: --error=error.out

Change to:

 # flux: --output={{output_file}}
 # flux: --error={{error_file}}

Ensure to pass the new parameters when rendering:

     cores=cores,
     memory_max=memory_max,
     run_time_max=run_time_max,
     dependency_list=dependency_list,
+    output_file=output_file,
+    error_file=error_file,
     **kwargs,

Committable suggestion was skipped due to low confidence.

# flux: -n {{cores}}
{%- if run_time_max %}
# flux: -t {{ [1, run_time_max // 60]|max }}
{%- endif %}
{%- if dependency %}
{%- for jobid in dependency %}
# flux: --dependency=afterok:{{jobid}}
{%- endfor %}
{%- endif %}
Comment on lines +20 to +24
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Inconsistency between template variable and render context variable

In the template, you use {%- if dependency %} and {%- for jobid in dependency %}, but in the render method, you pass dependency_list. This will cause an UndefinedError because dependency is not defined in the template context.

To fix this, replace dependency with dependency_list in the template:

 {%- if dependency %}
 {%- for jobid in dependency %}
 # flux: --dependency=afterok:{{jobid}}
 {%- endfor %}
 {%- endif %}

Change to:

 {%- if dependency_list %}
 {%- for jobid in dependency_list %}
 # flux: --dependency=afterok:{{jobid}}
 {%- endfor %}
 {%- endif %}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{%- if dependency %}
{%- for jobid in dependency %}
# flux: --dependency=afterok:{{jobid}}
{%- endfor %}
{%- endif %}
{%- if dependency_list %}
{%- for jobid in dependency_list %}
# flux: --dependency=afterok:{{jobid}}
{%- endfor %}
{%- endif %}


{{command}}
"""


class FluxCommands(SchedulerCommands):
Expand Down Expand Up @@ -51,3 +74,43 @@ def convert_queue_status(queue_status_output: str) -> pandas.DataFrame:
df.loc[df.status == "C", "status"] = "error"
df.loc[df.status == "CD", "status"] = "finished"
return df

def render_submission_template(
self,
command: str,
job_name: str = "pysqa",
working_directory: str = os.path.abspath("."),
cores: int = 1,
memory_max: Optional[int] = None,
run_time_max: Optional[int] = None,
dependency_list: Optional[List[int]] = None,
submission_template: Union[str, Template] = template,
**kwargs,
) -> str:
"""
Generate the job submission template.

Args:
command (str, optional): The command to be executed.
job_name (str, optional): The job name. Defaults to "pysqa".
working_directory (str, optional): The working directory. Defaults to ".".
cores (int, optional): The number of cores. Defaults to 1.
memory_max (int, optional): The maximum memory. Defaults to None.
run_time_max (int, optional): The maximum run time. Defaults to None.
dependency_list (list[int], optional): The list of dependency job IDs. Defaults to None.
submission_template (str): Submission script template pysqa.wrapper.flux.template

Returns:
str: The rendered job submission template.
"""
return super().render_submission_template(
command=command,
job_name=job_name,
working_directory=working_directory,
cores=cores,
memory_max=memory_max,
run_time_max=run_time_max,
dependency_list=dependency_list,
submission_template=submission_template,
**kwargs,
)
65 changes: 64 additions & 1 deletion pysqa/wrapper/lsf.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,13 @@
# Copyright (c) Max-Planck-Institut für Eisenforschung GmbH - Computational Materials Design (CM) Department
# Distributed under the terms of "New BSD License", see the LICENSE file.

import os
from typing import List, Optional, Union

import pandas
from jinja2 import Template

from pysqa.wrapper.generic import SchedulerCommands
from pysqa.wrapper.abstract import SchedulerCommands

__author__ = "Jan Janssen"
__copyright__ = (
Expand All @@ -18,6 +22,25 @@
__date__ = "Feb 9, 2019"


template = """\
#!/bin/bash
#BSUB -q queue
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Parameterize the queue name in the submission template

The queue name in the submission template is hardcoded as 'queue'. To make the template more flexible and reusable, consider adding a queue parameter to the render_submission_template method and updating the template to use this parameter.

Apply this diff to parameterize the queue name:

 #!/bin/bash
-#BSUB -q queue
+#BSUB -q {{queue}}
 #BSUB -J {{job_name}}

Update the render_submission_template method signature to include the queue parameter with a default value:

 def render_submission_template(
     command: str,
+    queue: str = "default",
     job_name: str = "pysqa",
     working_directory: str = os.path.abspath("."),
     cores: int = 1,
     memory_max: Optional[int] = None,
     run_time_max: Optional[int] = None,
     dependency_list: Optional[List[int]] = None,
     submission_template: Union[str, Template] = template,
     **kwargs,
 ) -> str:

Pass the queue parameter to the template rendering:

 return submission_template.render(
     command=command,
+    queue=queue,
     job_name=job_name,
     working_directory=working_directory,
     cores=cores,
     memory_max=memory_max,
     run_time_max=run_time_max,
     dependency_list=dependency_list,
     **kwargs,
 )

Committable suggestion was skipped due to low confidence.

#BSUB -J {{job_name}}
#BSUB -o time.out
#BSUB -n {{cores}}
#BSUB -cwd {{working_directory}}
#BSUB -e error.out
{%- if run_time_max %}
#BSUB -W {{run_time_max}}
{%- endif %}
{%- if memory_max %}
#BSUB -M {{memory_max}}
{%- endif %}

{{command}}
"""
Comment on lines +9 to +25
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ensure all necessary parameters are included in the submission template

The submission template currently does not include placeholders for parameters such as dependency_list and any additional parameters that might be provided via **kwargs. If these parameters are essential for the job submission script, consider incorporating them into the template.

For example, to handle dependencies:

 {%- if dependency_list %}
+#BSUB -w "{{ ' && '.join(dependency_list) }}"
 {%- endif %}

Ensure that any additional parameters supplied through **kwargs are appropriately used in the template to maximize the flexibility and usefulness of the method.

Committable suggestion was skipped due to low confidence.



class LsfCommands(SchedulerCommands):
@property
def submit_job_command(self) -> list[str]:
Expand Down Expand Up @@ -63,3 +86,43 @@ def convert_queue_status(queue_status_output: str) -> pandas.DataFrame:
df.loc[df.status == "RUN", "status"] = "running"
df.loc[df.status == "PEND", "status"] = "pending"
return df

def render_submission_template(
self,
command: str,
job_name: str = "pysqa",
working_directory: str = os.path.abspath("."),
cores: int = 1,
memory_max: Optional[int] = None,
run_time_max: Optional[int] = None,
dependency_list: Optional[List[int]] = None,
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Unused parameter dependency_list

The parameter dependency_list is accepted by the render_submission_template method but is not utilized within the method or the submission template. To avoid confusion, consider using this parameter in the template to handle job dependencies or remove it from the method signature if it's unnecessary.

If you intend to include job dependencies, you could modify the template to add dependency handling. For example:

 {%- if dependency_list %}
+#BSUB -w "ended({{ ' && ended('.join(dependency_list) }})"
 {%- endif %}

Ensure to convert dependency_list to a format suitable for the LSF -w option.

Committable suggestion was skipped due to low confidence.

submission_template: Union[str, Template] = template,
**kwargs,
) -> str:
"""
Generate the job submission template.

Args:
command (str, optional): The command to be executed.
job_name (str, optional): The job name. Defaults to "pysqa".
working_directory (str, optional): The working directory. Defaults to ".".
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Inconsistent default value for working_directory in the docstring

The default value for working_directory in the docstring is specified as ".", but the actual default in the code is os.path.abspath("."), which returns the absolute path of the current directory. Please update the docstring to accurately reflect the default value provided in the code.

Suggested update to the docstring:

-    working_directory (str, optional): The working directory. Defaults to ".".
+    working_directory (str, optional): The working directory. Defaults to the absolute path of the current directory.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
working_directory (str, optional): The working directory. Defaults to ".".
working_directory (str, optional): The working directory. Defaults to the absolute path of the current directory.

cores (int, optional): The number of cores. Defaults to 1.
memory_max (int, optional): The maximum memory. Defaults to None.
run_time_max (int, optional): The maximum run time. Defaults to None.
dependency_list (list[int], optional): The list of dependency job IDs. Defaults to None.
submission_template (str): Submission script template pysqa.wrapper.flux.template

Returns:
str: The rendered job submission template.
"""
return super().render_submission_template(
command=command,
job_name=job_name,
working_directory=working_directory,
cores=cores,
memory_max=memory_max,
run_time_max=run_time_max,
dependency_list=dependency_list,
submission_template=submission_template,
**kwargs,
)
61 changes: 60 additions & 1 deletion pysqa/wrapper/moab.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@
# Copyright (c) Max-Planck-Institut für Eisenforschung GmbH - Computational Materials Design (CM) Department
# Distributed under the terms of "New BSD License", see the LICENSE file.

from pysqa.wrapper.generic import SchedulerCommands
import os
from typing import List, Optional, Union

from jinja2 import Template

from pysqa.wrapper.abstract import SchedulerCommands

__author__ = "Jan Janssen"
__copyright__ = (
Expand All @@ -16,6 +21,20 @@
__date__ = "Feb 9, 2019"


template = """\
#!/bin/bash
#MSUB -N {{job_name}}
{%- if memory_max %}
#MSUB -l pmem={{ memory_max| int }}gb
{%- endif %}
{%- if run_time_max %}
#$ -l walltime={{run_time_max}}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct the resource directive prefix for consistency

The resource directive on line 31 uses #$, which is inconsistent with the other Moab directives that use #MSUB. This inconsistency may lead to the scheduler ignoring the line or causing unexpected behavior.

Please update the line to use the correct Moab directive prefix:

-#$ -l walltime={{run_time_max}}
+#MSUB -l walltime={{ run_time_max }}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#$ -l walltime={{run_time_max}}
#MSUB -l walltime={{ run_time_max }}

{%- endif %}

{{command}}
"""
Comment on lines +8 to +19
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Include the number of cores in the submission template

The cores parameter is accepted by the render_submission_template method but is not utilized in the submission script. To ensure the scheduler allocates the correct number of cores, include the appropriate resource directive in the submission template.

Add the following to the template to request the specified number of cores:

 {%- if run_time_max %}
 #MSUB -l walltime={{ run_time_max }}
 {%- endif %}
+{%- if cores %}
+#MSUB -l nodes=1:ppn={{ cores }}
+{%- endif %}

 {{command}}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
template = """\
#!/bin/bash
#MSUB -N {{job_name}}
{%- if memory_max %}
#MSUB -l pmem={{ memory_max| int }}gb
{%- endif %}
{%- if run_time_max %}
#$ -l walltime={{run_time_max}}
{%- endif %}
{{command}}
"""
template = """\
#!/bin/bash
#MSUB -N {{job_name}}
{%- if memory_max %}
#MSUB -l pmem={{ memory_max| int }}gb
{%- endif %}
{%- if run_time_max %}
#MSUB -l walltime={{run_time_max}}
{%- endif %}
{%- if cores %}
#MSUB -l nodes=1:ppn={{ cores }}
{%- endif %}
{{command}}
"""

⚠️ Potential issue

Handle job dependencies in the submission template

The dependency_list parameter is accepted but not utilized in the submission script. To properly manage job dependencies, include directives that specify the job IDs this job depends on.

Add the following lines to handle job dependencies:

 {%- if run_time_max %}
 #MSUB -l walltime={{ run_time_max }}
 {%- endif %}
+{%- if dependency_list %}
+#MSUB -l depend={{ dependency_list | join(':') }}
+{%- endif %}

 {{command}}

This inclusion will ensure that the job submission respects any specified dependencies.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
template = """\
#!/bin/bash
#MSUB -N {{job_name}}
{%- if memory_max %}
#MSUB -l pmem={{ memory_max| int }}gb
{%- endif %}
{%- if run_time_max %}
#$ -l walltime={{run_time_max}}
{%- endif %}
{{command}}
"""
template = """\
#!/bin/bash
#MSUB -N {{job_name}}
{%- if memory_max %}
#MSUB -l pmem={{ memory_max| int }}gb
{%- endif %}
{%- if run_time_max %}
#MSUB -l walltime={{run_time_max}}
{%- endif %}
{%- if dependency_list %}
#MSUB -l depend={{ dependency_list | join(':') }}
{%- endif %}
{{command}}
"""



class MoabCommands(SchedulerCommands):
@property
def submit_job_command(self) -> list[str]:
Expand Down Expand Up @@ -46,3 +65,43 @@ def get_queue_status_command(self) -> list[str]:
list[str]: The command to get the queue status.
"""
return ["mdiag", "-x"]

def render_submission_template(
self,
command: str,
job_name: str = "pysqa",
working_directory: str = os.path.abspath("."),
cores: int = 1,
memory_max: Optional[int] = None,
run_time_max: Optional[int] = None,
dependency_list: Optional[List[int]] = None,
submission_template: Union[str, Template] = template,
**kwargs,
) -> str:
"""
Generate the job submission template.

Args:
command (str, optional): The command to be executed.
job_name (str, optional): The job name. Defaults to "pysqa".
working_directory (str, optional): The working directory. Defaults to ".".
cores (int, optional): The number of cores. Defaults to 1.
memory_max (int, optional): The maximum memory. Defaults to None.
run_time_max (int, optional): The maximum run time. Defaults to None.
dependency_list (list[int], optional): The list of dependency job IDs. Defaults to None.
submission_template (str): Submission script template pysqa.wrapper.flux.template

Returns:
str: The rendered job submission template.
"""
return super().render_submission_template(
command=command,
job_name=job_name,
working_directory=working_directory,
cores=cores,
memory_max=memory_max,
run_time_max=run_time_max,
dependency_list=dependency_list,
submission_template=submission_template,
**kwargs,
)
Loading
Loading