8000 Batched crawl server (1/2) by Lama-Thematique · Pull Request #597 · mwmbl/mwmbl · GitHub
[go: up one dir, main page]
Skip to content

Batched crawl server (1/2) - #597

Merged
daoudclarke merged 1 commit into
mwmbl:mainfrom
Lama-Thematique:batched-crawl-results
Nov 6, 2026
Merged

Batched crawl server (1/2)#597
daoudclarke merged 1 commit into
mwmbl:mainfrom
Lama-Thematique:batched-crawl-results

Conversation

@Lama-Thematique
@Lama-Thematique Lama-Thematique commented Nov 5, 2026 •
edited by blueheron786
Loading
Copy link
Copy Markdown
Contributor

Backward compatible with older crawlers

Part 2: #598

@Lama-Thematique Lama-Thematique mentioned this pull request Nov 5, 2026
Merged
8000
@blueheron786
blueheron786 commented Nov 6, 2026
Copy link
Copy Markdown
Collaborator

Review of the server side. The response shape is additive, so older crawlers are unaffected. I found these issues:

1. The terms set from the request is unbounded and untrusted. Results.results has no max_length, and term is free-form text from any crawl-key holder. For each distinct term, post_results calls ranker.get_raw_results(term), which reads an index page, and appends the whole page to the response. One request with thousands of distinct terms makes thousands of page reads and a very large JSON body. Please cap the number of terms, or the total number of returned items, and return only the first N. The server already tokenises documents itself, so it could also derive terms from the documents rather than trust the client's term.

2. Nothing tests the new path. The only test change is the dry-run assertion. There is no test that include_new_items=True returns the items just indexed. There is also none for include_new_items=False leaving new_items as None, for blacklisted items being filtered out, or for two terms that share a page. Please add these to test/test_search_api_key.py.

3. The index is read right after index_documents() writes it. Can ranker.tiny_index (a separate handle, mmap) see that write? A test would show it.

4. The dry-run branch returns "new_items": None through the response model. That is fine, but the description on PostResultsResponse.new_items says "if include_new_items is False, this field is None". That is also true for dry-run, so please say so.

5. Small cleanups.

  • v2_router = Router(tags=["Crawler"]) # v2 is unused.
  • new_items: list[Document] = [] is declared before the if, so it is dead in the other branch.
  • The comment # maybe it can be directly obtained from is unfinished.
  • The two returns can be one: build the dict, then add new_items only when requested.
  • dataclasses.asdict(item) carries source, which RawDocument doesn't have. It is silently dropped today, so confirm that is intended.

6. batch.py now imports mwmbl.tinysearchengine.search. That pulls in mwmbl.models, quota and the rest of the search stack, and it needs django.setup() first. This works for crawl.py only because it calls django.setup() before importing. Importing mwmbl.crawler.batch on its own now raises AppRegistryNotReady. The crawler release workflow works hard to keep its imports light, so consider moving RawDocument into a light schema module.

@Lama-Thematique
Server side batched api
cb3ffc2 
No functional changes to the crawler.
@Lama-Thematique
Lama-Thematique commented Nov 6, 2026
Copy link
Copy Markdown
Contributor Author
  1. This was the case before, it was already unbonded, the response is large as well (that's new) and that's the goal, we prefer a large request than literally 300 small ones. If it ends up being a problem it should be added at that time.
  2. Test added
  3. I've though about that, it would be more efficient, but was not sure how to to actually get back the document stored without making the code an absolute mess, and there is plenty of reason why it's risky as well (such as code duplication or handling of blacklisted website) i decided against for now. It's still possible, but i would like @daoudclarke opinion on this.
  4. this is a bug, it's fixed
  5. fixed all, the drop of source is not an issue so that has not been changed
  6. I will leave this decision to @daoudclarke

@Lama-Thematique
Lama-Thematique commented Nov 6, 2026
Copy link
Copy Markdown
Contributor Author

also for the record source issue : #596

@daoudclarke
daoudclarke commented Nov 6, 2026
Copy link
Copy Markdown
Contributor

I think the concern for 3 is not efficiency but correctness, Claude is asking for a test to confirm the previous write is visible in the read because it's read with a different handle.

6 is concerning given the goal is to make the crawler more lightweight. I think it won't make a difference right now but it does hurt the long term goal, take a look at #580

It would be best to measure the crawler memory before and after the change just to be sure we're not making things worse, or do the suggested refactor.

This was referenced Nov 6, 2026
Open
Open
@daoudclarke
daoudclarke merged commit 4d26366 into mwmbl:main Nov 6, 2026
4 checks passed
@daoudclarke
daoudclarke commented Nov 6, 2026
Copy link
Copy Markdown
Contributor

The extra import only adds 2 MB of memory usage so merged as-is - also the write is read reliably immediately afterwards.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Lama-Thematique @blueheron786 @daoudclarke
0