fix(scraper): bounded per-host-polite parallel document scraping (#136)#145
fix(scraper): bounded per-host-polite parallel document scraping (#136)#145swinney wants to merge 17 commits into
Conversation
#136) introduce the two scrape-phase concurrency knobs on ScraperManager, parsed tolerantly (coerce to int, warn + default on junk, clamp to >= 1) and independent of the embedding-phase parallel_workers. defaults 8 / 4. document both in base-config.yaml with the postgres-pool caveat. repoint tasks.md at the active issue-136 change (prior 118 change is merged). Ralph-Task: 1.3 Implement the two-knob parsing in ScraperManager.__init__ to turn 1.1 and 1.2 green. Keep the diff to scraper_manager.py minimal.
introduce scrape_pool.HostLimiter, a lock-guarded dict[str, threading.Semaphore] exposed as a context manager keyed by hostname, so at most scrape_per_host_workers seeds targeting one host run concurrently. slots are released even when the guarded crawl raises, and distinct hosts never contend. tests assert on observed-peak concurrency (not wall-clock) plus a barrier-based distinct-host check and an exception-release check. Ralph-Task: 2.4 Implement `HostLimiter` (a lock-guarded `dict[str, threading.Semaphore]` plus a context manager keyed by hostname) to turn 2.1-2.3 green.
introduce scrape_pool.run_seeds, the standard-link seed pool: seeds fan across a ThreadPoolExecutor sized by workers, each per-seed crawl wrapped in the HostLimiter so the per-host cap holds on top of the global bound, and per-seed counts summed on the calling thread via as_completed. per-seed failures are isolated — logged and counted as zero while the rest of the batch runs — and workers=1 reproduces the sequential in-order path. tests assert on barrier-forced observed-peak concurrency (not wall-clock), the fail-open/all-fail cases, exact sum accumulation, and serial input-order execution. Ralph-Task: 3.7 Implement `run_seeds` (a `ThreadPoolExecutor` bounded by `workers`, each task wrapped in the `HostLimiter`, results summed on the calling thread via `as_completed`) to turn 3.1-3.6 green.
Concurrent seed crawls cannot share a single LinkScraper: crawl_iter resets and mutates per-instance visited_urls/seen_urls/page_data at the top of every call (scraper.py:194-196), so handing the manager's shared self.web_scraper to N threads makes that per-seed reset the *cause* of a race — one seed wipes another's in-flight frontier. Add ScraperManager._new_link_scraper(), a factory that returns a fresh LinkScraper per seed crawl, mirroring the shared scraper's verify_urls / enable_warnings construction args. self.web_scraper is now built through the same factory and stays in place for the sequential/selenium/SSO callers; the parallel path (wired in section 5) will pull a fresh instance per seed. Tests exercise the seam under forced concurrency (barrier-synchronized crawls) proving each crawl yields only its own URLs with no cross-contamination, and that per-worker scrapers inherit the manager's config. Ralph-Task: 4.3 Implement a scraper-factory seam on `ScraperManager` that returns a fresh `LinkScraper` per seed crawl, and route the parallel path through it. Leave `self.web_scraper` in place for the existing sequential/selenium callers.
_collect_links_from_urls now fans standard (non-selenium) seeds through run_seeds sized by scrape_workers/scrape_per_host_workers instead of the one-seed-at-a-time loop. Each concurrent seed crawl gets its own LinkScraper via the _new_link_scraper factory seam; _handle_standard_url takes an optional per-worker scraper and the sequential/SSO callers keep using self.web_scraper. The try/finally authenticator close is preserved. Ralph-Task: 5.2 Replace the for url in urls: loop at scraper_manager.py:346 with the run_seeds call. Preserve the existing try/finally authenticator close and the per-seed try/except fail-open already in _handle_standard_url.
Emit a single INFO summary from run_seeds after the pool drains, reporting seed count, effective workers, effective per-host cap, and elapsed wall-clock. Ralph-Task: 5.3 Write a failing test then implement the one-line completion summary: seed count, effective workers, effective per-host cap, elapsed wall-clock — logged exactly once after the pool drains.
Lock in the "selenium/SSO stays sequential" guarantee: _collect_sso_from_urls creates one shared authenticator and drives it through a plain sequential loop, so the observed peak concurrent use of that authenticator is exactly 1. If SSO collection were ever routed through the run_seeds pool, multiple worker threads would touch the shared authenticator at once and the peak would exceed 1. Ralph-Task: 6.1 Write a failing test that a shared authenticator is never used from two threads at once during SSO collection (assert observed peak concurrent authenticator use is 1).
Locks in the SSO/selenium authenticator lifecycle from the parallel-scraping spec: _collect_sso_from_urls releases its shared browser session exactly once via try/finally, whether every seed succeeds or one seed raises mid-loop. Ralph-Task: 6.2 Write a test that the authenticator is closed exactly once per run, including when a seed raises.
Ralph-Task: 6.3 Confirm by test that _collect_sso_from_urls (:363-404) is untouched by the pool and remains a plain sequential loop.
…e corpus (#136) Ralph-Task: 7.1 Write a failing test scraping the same fixture seeds twice — once at scrape_workers=1, once at scrape_workers=8 — asserting the set of persisted resource URLs is identical and both return the same total.
…#136) Task 7.1 checks the persisted URL *set* matches sequential vs parallel, which is blind to multiplicity. This adds a multiset assertion: under scrape_workers=8 each expected page is persisted exactly once and the returned total equals the persisted resource count, so a racing double-persist or leaked shared crawler revisit would surface as a URL with count > 1. Ralph-Task: 7.2 Write a test asserting no resource is dropped or duplicated under concurrency (persisted URL multiset has no unexpected repeats).
…it (#136) Ralph-Task: 7.3 Verify the pre-loop sitemap dedup (collect_all_from_config, :125-147) is unaffected — the pool receives an already-deduped list and does not re-dedup.
#136) Verify no diff to src/data_manager/vectorstore/manager.py or src/data_manager/collectors/scrapers/scraper.py vs origin/dev; the parallel-scrape change is confined to scrape_pool.py and scraper_manager.py. Ralph-Task: 8.1 Confirm the embedding phase is untouched: no diff to src/data_manager/vectorstore/manager.py, and no diff to src/data_manager/collectors/scrapers/scraper.py internals.
Add the two scrape-phase concurrency knobs to the data_manager Core Settings table in docs/docs/configuration.md, with a note clarifying they are independent of the embedding-phase parallel_workers and the Postgres pool caveat. Ralph-Task: 8.2 Update docs/ for the two new user-facing config knobs (project convention: user-facing config changes ship docs in the same change).
Ran scripts/gate.sh (black 24.10.0 + isort 6.0.1, pytest tests/unit/, diff-cover --fail-under=80 vs origin/dev): exit 0, 1247 passed / 1 xfailed, diff coverage 100% on scrape_pool.py and scraper_manager.py. Ralph-Task: 8.3 Run bash scripts/gate.sh and confirm exit 0 before every commit.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2e12858bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| started = time.perf_counter() | ||
|
|
||
| def _work(seed): | ||
| host = urlsplit(seed).netloc |
There was a problem hiding this comment.
When the seed list contains URL variants for the same host, such as https://EXAMPLE.com/a and https://example.com:443/b, using urlsplit(seed).netloc gives each spelling a different semaphore key, so the configured per-host cap can be bypassed and the same site can receive more concurrent crawls than scrape_per_host_workers allows. Key this on the parsed hostname normalized consistently with LinkScraper (and handle default ports/credentials deliberately) before acquiring the host slot.
Useful? React with 👍 / 👎.
|
|
||
| def _work(seed): | ||
| host = urlsplit(seed).netloc | ||
| with limiter.acquire(host): |
There was a problem hiding this comment.
When the input is grouped by host, for example many docs.example.edu seeds first and other hosts later, the executor's first workers can all enter _work and most of them block inside limiter.acquire(host) while waiting for the same host slot. Those blocked workers occupy the global pool, so queued seeds for other hosts cannot start even though their per-host capacity is free, making mixed-host scrapes depend on list order and potentially run nearly serially; avoid consuming a worker until a host slot is actually available or use per-host queues/fair scheduling.
Useful? React with 👍 / 👎.
| > `scrape_workers` above the Postgres connection pool max (`20`) requires raising | ||
| > that pool in tandem, or scrape workers will block on connection checkout. |
There was a problem hiding this comment.
When operators raise scrape_workers, this guidance says the workers will block on the Postgres connection pool and can be fixed by raising that pool, but the scraper persistence path uses PostgresCatalogService.upsert_resource(), which opens raw psycopg2.connect() connections rather than src/utils/connection_pool.py. In that configuration, raising the documented pool does not throttle or unblock scrape persistence; higher worker counts can instead create that many direct DB connections and fail/retry against Postgres, so either route these writes through the configured pool or document the actual DB limit.
Useful? React with 👍 / 👎.
| total_count = run_seeds( | ||
| urls, | ||
| _scrape_one_seed, | ||
| workers=self.scrape_workers, | ||
| per_host_workers=self.scrape_per_host_workers, | ||
| ) |
There was a problem hiding this comment.
When data_manager.processing.categorization.enabled is true, each scraped resource is classified inside persistence.persist_resource() before the file/catalog write, so running every seed through this pool also fan-outs those LLM invoke() calls up to scrape_workers concurrently. That concurrency is controlled by scrape-fetch knobs rather than any model/provider throttle, and provider rate-limit or client-thread-safety failures are caught as uncategorized, silently degrading metadata that was previously produced one document at a time; serialize or separately throttle the persistence/categorization step.
Useful? React with 👍 / 👎.
🤖 Automation cost — issue #136 (ultracode)
≈ $38.49 est. · tier: ultracode · 18 loop turns · propose+wrap-up: $34.04 |
1 participant
Closes #136.
Why
A clean re-ingest on the dev deployment spends ~19.5 minutes in a fully
single-threaded scrape loop for 815 catalog documents, while the downstream
embedding
phase already runs 32-way parallel. The scrape phase is almost entirely
network-bound I/O blocked on
requests.get, so that wall-clock cost is idlewaiting, not work. Re-ingest latency is now the slowest step in the
knowledge base
refresh loop.
Raw speed is not the goal — politeness is a hard constraint. The 815 docs
concentrate on a handful of hosts (
docs.rc.fas.harvard.edualone contributes212 pages), and a naive global fan-out would risk rate-limiting or an IP block
at our own documentation host — a strictly worse outcome than a slow run.
What changed
data_manager.scrape_workers, default8, anddata_manager.scrape_per_host_workers, default4), parsed with the sametolerant-fallback pattern as the existing
parallel_workersknob and keptseparate from the embedding pool — scrape and embed have different safe
ceilings.
scrape_pool.py: a per-host semaphore registry(
HostLimiter) and a bounded seed-pool executor (run_seeds), unit-testablein isolation with injected fakes.
ScraperManager._collect_links_from_urlsnow dispatches the standardnon-selenium path through
run_seedsinstead of a sequentialfor url in urls:loop, with each worker getting its ownLinkScraperinstance (the shared
self.web_scraperresetsvisited_urls/seen_urlsper crawl and would corrupt concurrent crawls).
shared, non-thread-safe browser session.
total_countaccumulates on the calling thread viaas_completed.effective workers, per-host cap, elapsed wall-clock.
Explicitly untouched
src/data_manager/vectorstore/manager.py(embedding is already parallel).src/data_manager/collectors/scrapers/scraper.py(LinkScraperinternals).Determinism
scrape_workers: 1reproduces the exact sequential path. Tests assert the setof persisted resource URLs is identical at
scrape_workers=1vs8, with nopage dropped or duplicated under concurrency.
Testing
bash scripts/gate.shpasses clean (black 24.10.0 + isort 6.0.1,pytest tests/unit/, diff-cover--fail-under=80vsorigin/dev). New suitescover config parsing,
HostLimiter,run_seedsbounds, per-worker isolation,dispatch wiring, SSO sequentiality, determinism, and sitemap-dedup invariance.
Docs
docs/docs/configuration.mdandsrc/cli/templates/base-config.yamldocumentboth knobs, including the note that raising
scrape_workersabove the Postgrespool max (20) requires raising that pool in tandem.
🤖 Generated with Claude Code