feat(sparql-anything): convert several chunks at once - #795
Merged
Conversation
map.sh runs an xargs -P pool over its chunks; the converter ran them one at a time, which on allCountries multiplies a job that already takes about a day. concurrency says how many to convert at once, over the chunks of every job in one pool, so a long places chunk and a short alternate-names one pack together rather than draining in phases. The output is concatenated by position, not by completion, so it stays in the order the jobs and their chunks were given. The first failure aborts the run: no further chunk is started, and the processes still going are stopped rather than left writing into a directory convert() is about to delete. The default is 1, and deliberately not derived from the CPU count or a memory limit. Each chunk is a JVM capped by `heap`, so a pool needs concurrency × heap on the machine the TASK RUNNER uses – which is not this process's machine once the runner is Docker or remote. The converter cannot see that machine, so the number belongs to the caller, who can do map.sh's arithmetic for the deployment they actually have. Workers pull from the queue one at a time rather than sharing a for...of over it: leaving a for-of early closes the iterator, so the first worker to give up would silently end the queue for the others.
…e failure Three ways the abort could go wrong, all of them found by review: - Stopping was unguarded. A task runner cannot stop what has already exited – DockerTaskRunner's stop() reads logs and stops the container, and both reject for one that is gone – so a stop failure replaced the conversion failure and rejected the join, leaving the other workers unawaited while the run directory was deleted underneath them. Stopping is best effort now. - A chunk could be started after the run had failed. The guard sits before two awaits, so a worker could pass it, then start a process while another chunk was failing; that process was not in the set the failure stopped, and the run waited out its whole conversion. Each worker now checks once its process exists, and stops it. - A DockerTaskRunner with a containerName force-removes any container of that name before starting a task, so chunks in parallel would destroy each other's containers. Documented as a warning; the runner needs a per-task name before that combination can work. The fake task runner's delays never applied: they were keyed by file name while the command names the output relative to the run directory, so every lookup missed and every delay was zero. Two tests that read as though they staged an ordering were passing on whatever order the event loop produced.
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
1 participant
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.
Must 1 of #782, the last of them.
map.shruns anxargs -Ppool over its chunks; the converter ran them one at a time. OnallCountriesthat is ~14 places chunks plus ~4 alternate-names chunks, run end to end, multiplying a job that already takes about a day.concurrencysays how many chunks to convert at once, over the chunks of every job in one pool – so a long places chunk and a short alternate-names one pack together instead of draining in phases, which is what #791 set up.The default is 1, on purpose
Each chunk is a JVM capped by
heap, so a pool needsconcurrency × heap– on the machine the task runner uses. That is not this process's machine once the runner is Docker or remote, soos.availableParallelism()and the Node process's cgroup limit describe the wrong host. #782 suggested a memory-aware default; the converter cannot see the memory it would need to be aware of.So the number belongs to the caller, who can do
map.sh's arithmetic – CPU count, capped by the memory limit, ~3 GB a worker – against the deployment they actually have. Left alone, chunks are converted one after another, exactly as before this PR.Order and failure
convert()is about to delete. Without that, a fail-fast abort would race its own cleanup, and a 2 GB JVM would keep running for minutes after the run had "failed".A dead branch that was load-bearing
The workers first shared one
for...ofover the queue. That passed its tests, and the coverage requirement then flagged the abort guard inside the loop as never executed – which was true, and the reason is that leaving afor...ofearly closes the iterator. The first worker to fail was silently ending the queue for every other worker, and the test asserting "the fourth chunk was never started" passed for that reason rather than the intended one.Workers now pull from the queue explicitly, one at a time, so the guard does the work and no worker's exit affects another's.
Tests
Five new: the pool reaches but does not exceed
concurrency; the default stays sequential; the output order survives out-of-order completion; a failure stops the chunks still running and starts no more; and aconcurrencythat is not a whole number of processes is rejected.