Skip to content

Commit

Permalink
Remove script_dir state from channels (#3714)
Browse files Browse the repository at this point in the history
This leaves channels as a stateless (and useless) object that can be
removed in a subsequent PR. That removal will change/break a lot of
config files so I want it to keep it separate from this attribute
removal.

# Changed Behaviour

Batch providers will now use their own script_dir attribute, rather than
the channel script_dir attribute. Prior to PR #3688 these attributes
were paths on different file systems: the submit side (for providers)
and the batch job execution side (for channels). PR #3688 removed
support for batch job commands to run somewhere that isn't the workflow
submit side, and so since then, these two attributes have been roughly
equivalent.

In some (obscure seeming cases) they might be different: if a channel is
shared between DFKs, then the script_dir used by providers in one DFK
will now no longer use the channel script dir set by the other DFK. This
was probably a bug anyway but I am noting it because this PR isn't
completely behaviour preserving.

## Type of change

- Code maintenance/cleanup
  • Loading branch information
benclifford authored Dec 3, 2024
1 parent a9a5b4b commit 76e5328
Show file tree
Hide file tree
Showing 11 changed files with 11 additions and 53 deletions.
21 changes: 2 additions & 19 deletions parsl/channels/base.py
Original file line number Diff line number Diff line change
@@ -1,22 +1,5 @@
from abc import ABCMeta, abstractproperty
from abc import ABCMeta


class Channel(metaclass=ABCMeta):
@abstractproperty
def script_dir(self) -> str:
''' This is a property. Returns the directory assigned for storing all internal scripts such as
scheduler submit scripts. This is usually where error logs from the scheduler would reside on the
channel destination side.
Args:
- None
Returns:
- Channel script dir
'''
pass

# DFK expects to be able to modify this, so it needs to be in the abstract class
@script_dir.setter
def script_dir(self, value: str) -> None:
pass
pass
23 changes: 1 addition & 22 deletions parsl/channels/local/local.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import logging
import os

from parsl.channels.base import Channel
from parsl.utils import RepresentationMixin
Expand All @@ -8,24 +7,4 @@


class LocalChannel(Channel, RepresentationMixin):
''' This is not even really a channel, since opening a local shell is not heavy
and done so infrequently that they do not need a persistent channel
'''

def __init__(self):
''' Initialize the local channel. script_dir is required by set to a default.
KwArgs:
- script_dir (string): Directory to place scripts
'''
self.script_dir = None

@property
def script_dir(self):
return self._script_dir

@script_dir.setter
def script_dir(self, value):
if value is not None:
value = os.path.abspath(value)
self._script_dir = value
pass
2 changes: 0 additions & 2 deletions parsl/dataflow/dflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -1151,8 +1151,6 @@ def add_executors(self, executors: Sequence[ParslExecutor]) -> None:
executor.provider.script_dir = os.path.join(self.run_dir, 'submit_scripts')
os.makedirs(executor.provider.script_dir, exist_ok=True)

executor.provider.channel.script_dir = executor.provider.script_dir

self.executors[executor.label] = executor
executor.start()
block_executors = [e for e in executors if isinstance(e, BlockProviderExecutor)]
Expand Down
2 changes: 1 addition & 1 deletion parsl/providers/condor/condor.py
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ def submit(self, command, tasks_per_node, job_name="parsl.condor"):

job_config = {}
job_config["job_name"] = job_name
job_config["submit_script_dir"] = self.channel.script_dir
job_config["submit_script_dir"] = self.script_dir
job_config["project"] = self.project
job_config["nodes"] = self.nodes_per_block
job_config["scheduler_options"] = scheduler_options
Expand Down
2 changes: 1 addition & 1 deletion parsl/providers/grid_engine/grid_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ def get_configs(self, command, tasks_per_node):
self.nodes_per_block, tasks_per_node))

job_config = {}
job_config["submit_script_dir"] = self.channel.script_dir
job_config["submit_script_dir"] = self.script_dir
job_config["nodes"] = self.nodes_per_block
job_config["walltime"] = self.walltime
job_config["scheduler_options"] = self.scheduler_options
Expand Down
2 changes: 1 addition & 1 deletion parsl/providers/lsf/lsf.py
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ def submit(self, command, tasks_per_node, job_name="parsl.lsf"):
logger.debug("Requesting one block with {} nodes".format(self.nodes_per_block))

job_config = {}
job_config["submit_script_dir"] = self.channel.script_dir
job_config["submit_script_dir"] = self.script_dir
job_config["nodes"] = self.nodes_per_block
job_config["tasks_per_node"] = tasks_per_node
job_config["walltime"] = wtime_to_minutes(self.walltime)
Expand Down
2 changes: 1 addition & 1 deletion parsl/providers/pbspro/pbspro.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ def submit(self, command, tasks_per_node, job_name="parsl"):
)

job_config = {}
job_config["submit_script_dir"] = self.channel.script_dir
job_config["submit_script_dir"] = self.script_dir
job_config["nodes_per_block"] = self.nodes_per_block
job_config["ncpus"] = self.cpus_per_node
job_config["walltime"] = self.walltime
Expand Down
6 changes: 3 additions & 3 deletions parsl/providers/slurm/slurm.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import os
import re
import time
from typing import Optional
from typing import Any, Dict, Optional

import typeguard

Expand Down Expand Up @@ -286,8 +286,8 @@ def submit(self, command: str, tasks_per_node: int, job_name="parsl.slurm") -> s

logger.debug("Requesting one block with {} nodes".format(self.nodes_per_block))

job_config = {}
job_config["submit_script_dir"] = self.channel.script_dir
job_config: Dict[str, Any] = {}
job_config["submit_script_dir"] = self.script_dir
job_config["nodes"] = self.nodes_per_block
job_config["tasks_per_node"] = tasks_per_node
job_config["walltime"] = wtime_to_minutes(self.walltime)
Expand Down
2 changes: 1 addition & 1 deletion parsl/providers/torque/torque.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ def submit(self, command, tasks_per_node, job_name="parsl.torque"):

job_config = {}
# TODO : script_path might need to change to accommodate script dir set via channels
job_config["submit_script_dir"] = self.channel.script_dir
job_config["submit_script_dir"] = self.script_dir
job_config["nodes"] = self.nodes_per_block
job_config["task_blocks"] = self.nodes_per_block * tasks_per_node
job_config["nodes_per_block"] = self.nodes_per_block
Expand Down
1 change: 0 additions & 1 deletion parsl/tests/test_providers/test_pbspro_template.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ def test_submit_script_basic(tmp_path):
queue="debug", channel=LocalChannel()
)
provider.script_dir = tmp_path
provider.channel.script_dir = tmp_path
job_id = str(random.randint(55000, 59000))
provider.execute_wait = mock.Mock(spec=PBSProProvider.execute_wait)
provider.execute_wait.return_value = (0, job_id, "")
Expand Down
1 change: 0 additions & 1 deletion parsl/tests/test_providers/test_slurm_template.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ def test_submit_script_basic(tmp_path):
partition="debug", channel=LocalChannel()
)
provider.script_dir = tmp_path
provider.channel.script_dir = tmp_path
job_id = str(random.randint(55000, 59000))
provider.execute_wait = mock.MagicMock(spec=SlurmProvider.execute_wait)
provider.execute_wait.return_value = (0, f"Submitted batch job {job_id}", "")
Expand Down

0 comments on commit 76e5328

Please sign in to comment.