fix: skip the process pool when only one worker is available#2187
Closed
Souptik96 wants to merge 1 commit into
Closed
fix: skip the process pool when only one worker is available#2187Souptik96 wants to merge 1 commit into
Souptik96 wants to merge 1 commit into
Conversation
…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.
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses item 2 of #2173.
Item 2: skip the pool when
max_workersresolves to 1_extract_parallelspawned aProcessPoolExecutorwhenever 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 defaultthere 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 fallsback to
_extract_sequentialin-process when_extract_parallelreturnsFalse:Item 1 (
graphify watchtimeout): needs a decision first, so it's not hereThe issue says watch's timeout "is guarded on
signal.SIGALRMand is therefore a no-op onWindows", and asks for the same
else: threading.Timer -> os._exitfallback. Checkingv8,that premise doesn't hold:
graphify/watch.pycontains noSIGALRM, nosignal.alarm, andno
threading.Timerat all —watch()calls_rebuild_code(watch_path)straight from itsdebounce loop. There is no
elsebranch to add, on Windows or anywhere; the timeout is absenton 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),
watchis a long-running foreground process,and an
os._exit(1)watchdog would kill the user's watcher on a slow-but-healthy rebuild of alarge 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
Confirmed the new test catches the bug — same test with
graphify/extract.pyreverted tov8:Full suite (this change touches the extraction core, so I ran everything rather than a subset):
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 countonly 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, andbash-dependent tests.I have no Linux/macOS box to confirm they're green there, so please read that number in that
light.