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

DM-41832: Use spawn as start method, deprecate fork option #274

Merged
merged 6 commits into from
Nov 22, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
4 changes: 2 additions & 2 deletions .github/workflows/build.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
python-version: ["3.10", "3.11"]
python-version: ["3.11"]

steps:
- uses: actions/checkout@v3
Expand Down Expand Up @@ -79,7 +79,7 @@ jobs:
- name: Set up Python
uses: actions/setup-python@v4
with:
python-version: "3.10"
python-version: "3.11"

- name: Install dependencies
run: |
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/build_docs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ jobs:
- name: Set up Python
uses: actions/setup-python@v3
with:
python-version: '3.10'
python-version: '3.11'
cache: "pip"
cache-dependency-path: "setup.cfg"

Expand All @@ -35,7 +35,7 @@ jobs:
run: pip install --no-deps -v .

- name: Install documenteer
run: pip install 'documenteer[pipelines]<0.8'
run: pip install 'documenteer[pipelines]>=0.8'

- name: Build documentation
working-directory: ./doc
Expand Down
6 changes: 3 additions & 3 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v4.4.0
rev: v4.5.0
hooks:
- id: check-yaml
- id: end-of-file-fixer
- id: trailing-whitespace
- id: check-toml
- repo: https://github.com/psf/black-pre-commit-mirror
rev: 23.9.1
rev: 23.11.0
hooks:
- id: black
# It is recommended to specify the latest version of Python
Expand All @@ -22,6 +22,6 @@ repos:
name: isort (python)
- repo: https://github.com/astral-sh/ruff-pre-commit
# Ruff version.
rev: v0.0.289
rev: v0.1.6
hooks:
- id: ruff
3 changes: 3 additions & 0 deletions doc/changes/DM-41832.removal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Support for fork option in `pipetask run` has been removed as unsafe.
Default start option now is `spawn`, `forkserver` is also available.
The `fork` option is still present in CLI for compatibility, but is deprecated and replaced by `spawn` if specified.
5 changes: 4 additions & 1 deletion python/lsst/ctrl/mpexec/cli/opt/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,10 @@
"--start-method",
default=None,
type=click.Choice(choices=["spawn", "fork", "forkserver"]),
help="Multiprocessing start method, default is platform-specific.",
help=(
"Multiprocessing start method, default is platform-specific. "
"Fork method is now deprecated, spawn is used instead if fork is selected."
andy-slac marked this conversation as resolved.
Show resolved Hide resolved
),
)


Expand Down
5 changes: 5 additions & 0 deletions python/lsst/ctrl/mpexec/cli/script/run.py
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,11 @@
function and pass all the option kwargs to each of the script functions
which ignore these unused kwargs.
"""
# Fork option still exists for compatibility but we use spawn instead.
if start_method == "fork":
start_method = "spawn"
_log.warning("Option --start-method=fork is deprecated and unsafe, will use spawn instead.")

Check warning on line 188 in python/lsst/ctrl/mpexec/cli/script/run.py

View check run for this annotation

Codecov / codecov/patch

python/lsst/ctrl/mpexec/cli/script/run.py#L187-L188

Added lines #L187 - L188 were not covered by tests
andy-slac marked this conversation as resolved.
Show resolved Hide resolved

args = SimpleNamespace(
pdb=pdb,
graph_fixup=graph_fixup,
Expand Down
8 changes: 8 additions & 0 deletions python/lsst/ctrl/mpexec/cli/script/run_qbb.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,13 @@
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.

import logging
from types import SimpleNamespace

from ... import CmdLineFwk, TaskFactory

_log = logging.getLogger(__name__)


def run_qbb(
butler_config: str,
Expand Down Expand Up @@ -97,6 +100,11 @@
implies no limit. The string can be either a single integer (implying
units of MB) or a combination of number and unit.
"""
# Fork option still exists for compatibility but we use spawn instead.
if start_method == "fork":
start_method = "spawn"
_log.warning("Option --start-method=fork is deprecated and unsafe, will use spawn instead.")

Check warning on line 106 in python/lsst/ctrl/mpexec/cli/script/run_qbb.py

View check run for this annotation

Codecov / codecov/patch

python/lsst/ctrl/mpexec/cli/script/run_qbb.py#L105-L106

Added lines #L105 - L106 were not covered by tests
andy-slac marked this conversation as resolved.
Show resolved Hide resolved

args = SimpleNamespace(
butler_config=butler_config,
qgraph=qgraph,
Expand Down
16 changes: 8 additions & 8 deletions python/lsst/ctrl/mpexec/mpGraphExecutor.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ def terminated(self) -> bool:
def start(
self,
quantumExecutor: QuantumExecutor,
startMethod: Literal["spawn"] | Literal["fork"] | Literal["forkserver"] | None = None,
startMethod: Literal["spawn"] | Literal["forkserver"],
) -> None:
"""Start process which runs the task.

Expand All @@ -119,11 +119,13 @@ def start(
logConfigState = CliLog.configState

mp_ctx = multiprocessing.get_context(startMethod)
self.process = mp_ctx.Process(
self.process = mp_ctx.Process( # type: ignore[attr-defined]
target=_Job._executeJob,
args=(quantumExecutor, taskDef, quantum_pickle, logConfigState, snd_conn),
name=f"task-{self.qnode.quantum.dataId}",
)
# mypy is getting confused by multiprocessing.
assert self.process is not None
self.process.start()
self.started = time.time()
self._state = JobState.RUNNING
Expand Down Expand Up @@ -268,7 +270,7 @@ def submit(
self,
job: _Job,
quantumExecutor: QuantumExecutor,
startMethod: Literal["spawn"] | Literal["fork"] | Literal["forkserver"] | None = None,
startMethod: Literal["spawn"] | Literal["forkserver"],
) -> None:
"""Submit one more job for execution

Expand Down Expand Up @@ -380,7 +382,7 @@ def __init__(
timeout: float,
quantumExecutor: QuantumExecutor,
*,
startMethod: Literal["spawn"] | Literal["fork"] | Literal["forkserver"] | None = None,
startMethod: Literal["spawn"] | Literal["forkserver"] | None = None,
failFast: bool = False,
pdb: str | None = None,
executionGraphFixup: ExecutionGraphFixup | None = None,
Expand All @@ -393,11 +395,9 @@ def __init__(
self.executionGraphFixup = executionGraphFixup
self.report: Report | None = None

# We set default start method as spawn for MacOS and fork for Linux;
# None for all other platforms to use multiprocessing default.
Copy link
Member

Choose a reason for hiding this comment

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

Is fork the multiprocessing default then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fork is default for Linux, spawn is default for MacOS and Windows.

# We set default start method as spawn for all platforms.
if startMethod is None:
methods = dict(linux="fork", darwin="spawn")
startMethod = methods.get(sys.platform) # type: ignore
startMethod = "spawn"
self.startMethod = startMethod

def execute(self, graph: QuantumGraph) -> None:
Expand Down
Loading