Skip to content

fix: skip the process pool when only one worker is available#2187

Closed
Souptik96 wants to merge 1 commit into
Graphify-Labs:v8from
Souptik96:fix/2173-skip-single-worker-pool
Closed

fix: skip the process pool when only one worker is available#2187
Souptik96 wants to merge 1 commit into
Graphify-Labs:v8from
Souptik96:fix/2173-skip-single-worker-pool

Conversation

@Souptik96

Copy link
Copy Markdown
Contributor

Addresses item 2 of #2173.

Item 2: skip the pool when max_workers resolves to 1

_extract_parallel spawned a ProcessPoolExecutor whenever there were
>= _PARALLEL_THRESHOLD (20) uncached files, regardless of the resolved worker count.
With one worker that buys no parallelism — it still pays a process spawn plus an IPC round
trip per file, and it's the one residual case where the parent's rebuild watchdog
(os._exit) can orphan a worker mid-task.

Since the Windows post-commit hook exports GRAPHIFY_MAX_WORKERS=1, this was the default
there for any rebuild touching 20+ uncached files.

The gate goes in after the env override and the win32/floor clamps, so it sees the resolved
count, and it returns False — reusing the existing contract, since the caller already falls
back to _extract_sequential in-process when _extract_parallel returns False:

ran_parallel = False
if parallel and len(uncached_work) >= _PARALLEL_THRESHOLD:
    ran_parallel = _extract_parallel(...)
if not ran_parallel:
    _extract_sequential(...)

Item 1 (graphify watch timeout): needs a decision first, so it's not here

The issue says watch's timeout "is guarded on signal.SIGALRM and is therefore a no-op on
Windows", and asks for the same else: threading.Timer -> os._exit fallback. Checking v8,
that premise doesn't hold: graphify/watch.py contains no SIGALRM, no signal.alarm, and
no threading.Timer at all
watch() calls _rebuild_code(watch_path) straight from its
debounce loop. There is no else branch to add, on Windows or anywhere; the timeout is absent
on every platform.

So implementing it means introducing a rebuild watchdog to watch, not fixing a
platform-conditional one. That's a behaviour change I didn't want to make unilaterally: unlike
the post-commit hook (short-lived, one rebuild), watch is a long-running foreground process,
and an os._exit(1) watchdog would kill the user's watcher on a slow-but-healthy rebuild of a
large repo. Reasonable options — abort the rebuild and keep watching, use a longer/separate
timeout for watch, or make it opt-in — are all product calls.

Happy to send that as a follow-up in whichever shape you prefer; just say which semantics you
want for a watch rebuild that exceeds the timeout.

Testing

$ uv run pytest tests/test_extract.py -q -k "skips_pool_when_max_workers_is_one or still_spawns_pool_for_multiple_workers"
2 passed, 149 deselected, 1 warning in 0.95s

Confirmed the new test catches the bug — same test with graphify/extract.py reverted to v8:

FAILED tests/test_extract.py::test_extract_parallel_skips_pool_when_max_workers_is_one
  - AssertionError: ProcessPoolExecutor must not be constructed for 1 worker

Full suite (this change touches the extraction core, so I ran everything rather than a subset):

$ uv run pytest tests/ -q
45 failed, 3591 passed, 15 skipped, 4 warnings in 413.39s
$ uv run ruff check --config pyproject.toml graphify/extract.py tests/test_extract.py
All checks passed!

$ uv run python -m tools.skillgen --check
check OK: 134 artifact(s) match committed output and expected/.

Those 45 failures are pre-existing on Windows, not from this PR. My baseline on unmodified
v8 (66d8110) is 45 failed, 3578 passed, 15 skipped — the same 45, and the passed count
only moves up. (My local tree also carried the two new tests from my #2166 branch during that
run, which is why passed goes 3578 -> 3591 rather than +2.) The pre-existing failures are
Windows-specific: WinError 2/32, POSIX path-separator assertions, and bash-dependent tests.
I have no Linux/macOS box to confirm they're green there, so please read that number in that
light.

…y-Labs#2173)

`_extract_parallel` spawned a ProcessPoolExecutor whenever there were at least
_PARALLEL_THRESHOLD (20) uncached files, even when the resolved worker count was
1. A one-worker pool buys no parallelism: it still pays a process spawn plus an
IPC round trip per file, and it is the one residual case where the parent's
rebuild watchdog (os._exit) can orphan a worker that is mid-task.

The Windows post-commit hook exports GRAPHIFY_MAX_WORKERS=1, so this was the
default there for any rebuild touching 20+ uncached files.

Gate the pool on the resolved worker count -- after the GRAPHIFY_MAX_WORKERS
override and the win32/floor clamps -- and return False when it is 1. That reuses
the existing contract: the caller already falls back to `_extract_sequential`
in-process when `_extract_parallel` returns False.

Tests: no pool is constructed with GRAPHIFY_MAX_WORKERS=1 and 25 uncached files,
and a multi-worker run still takes the pool path.

Only item 2 of Graphify-Labs#2173 is addressed here. Item 1 (the `graphify watch` rebuild
timeout) needs a maintainer decision first: `watch()` currently arms no timeout
at all on any platform -- there is no signal.SIGALRM branch in graphify/watch.py
to add an `else` to -- so applying the hook's shape means adding a watchdog that
os._exit(1)s a long-running foreground watcher on a slow-but-healthy rebuild.
That is a behaviour change rather than a Windows-compat fix, so it is left out of
this PR.
@safishamsi

Copy link
Copy Markdown
Collaborator

Thanks @Souptik96. Shipped in v0.9.27 (cherry-picked to v8). Closed-unmerged here, but it's in the release: https://github.com/Graphify-Labs/graphify/releases/tag/v0.9.27

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants