diff --git a/datasets/amfv_datasets/scraping/__init__.py b/datasets/amfv_datasets/scraping/__init__.py index cd7cdb06..4ed0d214 100644 --- a/datasets/amfv_datasets/scraping/__init__.py +++ b/datasets/amfv_datasets/scraping/__init__.py @@ -1,4 +1,8 @@ -"""Web scraping helpers and source-specific scrapers.""" +"""Web scraping helpers and source-specific scrapers. + +Only the shared scraping contract is re-exported here. Import a source's own +symbols from its module, so registering a source touches one file. +""" from amfv_datasets.scraping.base import ( USER_AGENT, @@ -8,7 +12,7 @@ default_client, scrape_listing_documents, ) -from amfv_datasets.scraping.cli import OutputFormat, ScraperSource +from amfv_datasets.scraping.cli import ALL_SOURCES, SCRAPERS, OutputFormat, Scraper from amfv_datasets.scraping.html import ( LinkMode, absolute_unique_urls, @@ -17,38 +21,22 @@ first_matching_urls, html_to_markdown, ) -from amfv_datasets.scraping.nice import ( - GuidanceListingPage, - GuidanceRef, - NiceFetchError, - build_guideline_text, - guidance_ref_from_url, - list_published_guidance, - scrape_guideline, - scrape_nice, -) __all__ = [ - "GuidanceRef", - "GuidanceListingPage", + "ALL_SOURCES", "LinkMode", - "NiceFetchError", "OutputFormat", + "SCRAPERS", "ScrapeError", "ScrapeRun", "ScrapedDocument", - "ScraperSource", + "Scraper", "USER_AGENT", "absolute_unique_urls", - "build_guideline_text", "clean_text", - "document_title", "default_client", + "document_title", "first_matching_urls", - "guidance_ref_from_url", "html_to_markdown", - "list_published_guidance", - "scrape_guideline", "scrape_listing_documents", - "scrape_nice", ] diff --git a/datasets/amfv_datasets/scraping/base.py b/datasets/amfv_datasets/scraping/base.py index 0708ee87..9c278e08 100644 --- a/datasets/amfv_datasets/scraping/base.py +++ b/datasets/amfv_datasets/scraping/base.py @@ -69,12 +69,12 @@ def default_client( return httpx.Client(headers=client_headers, timeout=timeout, follow_redirects=follow_redirects) -def scrape_listing_documents[ListingItemT]( +def scrape_listing_documents[ClientT, ListingItemT]( *, documents: int | None, - client_factory: Callable[[], AbstractContextManager[httpx.Client]], - list_page: Callable[[httpx.Client, int], Iterable[ListingItemT]], - scrape_item: Callable[[httpx.Client, ListingItemT], ScrapedDocument], + client_factory: Callable[[], AbstractContextManager[ClientT]], + list_page: Callable[[ClientT, int], Iterable[ListingItemT]], + scrape_item: Callable[[ClientT, ListingItemT], ScrapedDocument | None], document_delay_seconds: float = 5.0, first_page_items: Iterable[ListingItemT] | None = None, ) -> Iterable[ScrapedDocument]: @@ -83,9 +83,11 @@ def scrape_listing_documents[ListingItemT]( Args: documents: Number of documents to scrape. When unset, listing pages are fetched until a page returns no items (default: None). - client_factory: Factory returning a context-managed HTTP client. + client_factory: Factory returning a context-managed client, passed to + `list_page` and `scrape_item` unchanged. list_page: Function that lists source-specific items for a page. - scrape_item: Function that scrapes one listed item into a document. + scrape_item: Function that scrapes one listed item into a document. It + may return None to skip a discovered item that is not a document. document_delay_seconds: Delay before scraping each document after the first one (default: 5.0). first_page_items: Already-fetched first listing page items. When set, @@ -99,6 +101,7 @@ def scrape_listing_documents[ListingItemT]( with client_factory() as client: page = 1 scraped = 0 + attempted = 0 page_items = list(first_page_items) if first_page_items is not None else None while documents is None or scraped < documents: if page_items is None: @@ -111,9 +114,13 @@ def scrape_listing_documents[ListingItemT]( for item in items: if documents is not None and scraped >= documents: break - if scraped and document_delay_seconds: + if attempted and document_delay_seconds: time.sleep(document_delay_seconds) - yield scrape_item(client, item) + document = scrape_item(client, item) + attempted += 1 + if document is None: + continue + yield document scraped += 1 page += 1 diff --git a/datasets/amfv_datasets/scraping/cli.py b/datasets/amfv_datasets/scraping/cli.py index e8aece34..acdcd050 100644 --- a/datasets/amfv_datasets/scraping/cli.py +++ b/datasets/amfv_datasets/scraping/cli.py @@ -8,8 +8,9 @@ from collections.abc import Iterable from dataclasses import asdict from enum import StrEnum +from itertools import chain from pathlib import Path -from typing import Annotated, TextIO +from typing import Annotated, Protocol, TextIO import typer from rich.console import Console @@ -29,11 +30,20 @@ from amfv_datasets.scraping.nice import scrape_nice -class ScraperSource(StrEnum): - """Supported scraper sources.""" +class Scraper(Protocol): + """Entry point a source module exposes to run one scrape.""" - ALL = "all" - NICE = "nice" + def __call__(self, *, documents: int | None, link_mode: LinkMode, url: str | None) -> ScrapeRun: + """Scrape a source into a run of documents.""" + ... + + +ALL_SOURCES = "all" + +SCRAPERS: dict[str, Scraper] = { + "nice": scrape_nice, +} +"""Scraper entry point by source name. Adding a source is an import and an entry here.""" class OutputFormat(StrEnum): @@ -48,7 +58,7 @@ class OutputFormat(StrEnum): def scrape_documents( - source: ScraperSource, + source: str, *, documents: int | None, link_mode: LinkMode, @@ -57,8 +67,8 @@ def scrape_documents( """Configure a scrape for a source. Args: - source: Scraper source to run. Use `ScraperSource.ALL` to run every - implemented source. + source: Scraper source to run. Use `ALL_SOURCES` to run every registered + source. documents: Number of documents to scrape. When unset, each source runs until it is exhausted (default: None). link_mode: Whether links are kept as markdown links or stripped to their @@ -68,13 +78,11 @@ def scrape_documents( if documents is not None and documents < 1: raise ValueError(f"documents must be at least 1; got {documents}") - for selected_source in _expand_source(source): - match selected_source: - case ScraperSource.NICE: - return scrape_nice(documents=documents, link_mode=link_mode, url=url) - case ScraperSource.ALL: - raise AssertionError("expanded source cannot be all") - raise AssertionError(f"unsupported source: {source}") + scrape_runs = [SCRAPERS[name](documents=documents, link_mode=link_mode, url=url) for name in _expand_source(source)] + if len(scrape_runs) == 1: + return scrape_runs[0] + total = None if any(run.total is None for run in scrape_runs) else sum(run.total or 0 for run in scrape_runs) + return ScrapeRun(documents=chain.from_iterable(run.documents for run in scrape_runs), total=total) def write_jsonl(documents: Iterable[ScrapedDocument], output: TextIO) -> int: @@ -123,15 +131,17 @@ def write_markdown_files(documents: Iterable[ScrapedDocument], output_path: Path return count -def _expand_source(source: ScraperSource) -> tuple[ScraperSource, ...]: - if source is ScraperSource.ALL: - return (ScraperSource.NICE,) +def _expand_source(source: str) -> tuple[str, ...]: + if source == ALL_SOURCES: + return tuple(SCRAPERS) + if source not in SCRAPERS: + raise typer.BadParameter(f"unknown source {source!r}; choose from {', '.join([ALL_SOURCES, *SCRAPERS])}") return (source,) @app.command(help="Run a scraper and write the scraped documents.") def run( - source: Annotated[ScraperSource, typer.Option("--source", help="Scraper source to run.")], + source: Annotated[str, typer.Option("--source", help=f"Scraper source to run: {', '.join([ALL_SOURCES, *SCRAPERS])}.")], # noqa: E501 url: Annotated[str | None, typer.Option("--url", help="Source URL to scrape as a single document.")] = None, documents: Annotated[str, typer.Option("--documents", help="Number of documents to scrape, or 'all'.")] = "1", link_mode: Annotated[LinkMode, typer.Option("--links", help="Whether to keep markdown links or strip links to text.")] = LinkMode.KEEP, # noqa: E501 @@ -168,7 +178,7 @@ def run( if output_path is None: raise typer.BadParameter("--output is required when --format markdown") count = write_markdown_files(scraped_documents, output_path) - target = url or source.value + target = url or source typer.echo(f"scraped {count} documents from {target}", err=True) @@ -248,8 +258,10 @@ def main() -> None: __all__ = [ + "ALL_SOURCES", + "SCRAPERS", "OutputFormat", - "ScraperSource", + "Scraper", "app", "LinkMode", "main", diff --git a/datasets/amfv_datasets/scraping/html.py b/datasets/amfv_datasets/scraping/html.py index 2bd1c968..2e670d0a 100644 --- a/datasets/amfv_datasets/scraping/html.py +++ b/datasets/amfv_datasets/scraping/html.py @@ -118,7 +118,10 @@ def html_to_markdown( def _absolutize_links(html_text: str, *, base_url: str) -> str: root = lxml_html.fragment_fromstring(html_text, create_parent="div") for link in root.xpath(".//a[@href]"): - link.set("href", urljoin(base_url, link.get("href"))) + href = link.get("href") + link.set("href", href if href.startswith("#") else urljoin(base_url, href)) + for image in root.xpath(".//img[@src]"): + image.set("src", urljoin(base_url, image.get("src"))) return "".join(lxml_html.tostring(child, encoding="unicode") for child in root) diff --git a/datasets/test/test_scraping_base.py b/datasets/test/test_scraping_base.py index eed3c838..2201e4de 100644 --- a/datasets/test/test_scraping_base.py +++ b/datasets/test/test_scraping_base.py @@ -149,3 +149,43 @@ def scrape_item(client: httpx.Client, item: str) -> ScrapedDocument: assert [document.external_id for document in documents] == ["item-1", "item-2", "item-3"] assert delays == [5.0, 5.0] + + +def test_scrape_listing_documents_skips_non_documents_without_consuming_limit( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Skipped listing entries are delayed but do not count as returned documents.""" + delays: list[float] = [] + + class _FakeClient: + def __enter__(self) -> "_FakeClient": + return self + + def __exit__(self, *args: object) -> None: + return None + + def client_factory(): + return _FakeClient() + + def list_page(client: httpx.Client, page: int) -> list[str]: + return ["skip", "item-1", "item-2"] if page == 1 else [] + + def scrape_item(client: httpx.Client, item: str) -> ScrapedDocument | None: + if item == "skip": + return None + return ScrapedDocument("test", item, item, f"https://example.org/{item}", "content") + + monkeypatch.setattr(base.time, "sleep", delays.append) + + documents = list( + scrape_listing_documents( + documents=2, + client_factory=client_factory, + list_page=list_page, + scrape_item=scrape_item, + document_delay_seconds=5.0, + ) + ) + + assert [document.external_id for document in documents] == ["item-1", "item-2"] + assert delays == [5.0, 5.0] diff --git a/datasets/test/test_scraping_cli.py b/datasets/test/test_scraping_cli.py index 44f714fa..bb24a7f6 100644 --- a/datasets/test/test_scraping_cli.py +++ b/datasets/test/test_scraping_cli.py @@ -9,7 +9,9 @@ from amfv_datasets.scraping.base import ScrapedDocument, ScrapeRun from amfv_datasets.scraping.cli import ( - ScraperSource, + ALL_SOURCES, + SCRAPERS, + _expand_source, app, write_huggingface_dataset, write_jsonl, @@ -65,13 +67,13 @@ def test_cli_run_writes_jsonl_to_stdout(monkeypatch: pytest.MonkeyPatch) -> None runner = CliRunner() def fake_scrape_documents( - source: ScraperSource, + source: str, *, documents: int | None, link_mode: LinkMode, url: str | None = None, ) -> ScrapeRun: - assert source is ScraperSource.NICE + assert source == "nice" assert documents == 3 assert link_mode is LinkMode.STRIP assert url is None @@ -100,7 +102,7 @@ def test_cli_run_can_disable_progress_for_file_output(monkeypatch: pytest.Monkey runner = CliRunner() def fake_scrape_documents( - source: ScraperSource, + source: str, *, documents: int | None, link_mode: LinkMode, @@ -132,7 +134,7 @@ def test_cli_run_uses_progress_by_default(monkeypatch: pytest.MonkeyPatch, tmp_p progress_calls = 0 def fake_scrape_documents( - source: ScraperSource, + source: str, *, documents: int | None, link_mode: LinkMode, @@ -182,7 +184,7 @@ def fake_progress( assert total == 1 yield from documents - monkeypatch.setattr("amfv_datasets.scraping.cli.scrape_nice", fake_scrape_nice) + monkeypatch.setitem(SCRAPERS, "nice", fake_scrape_nice) monkeypatch.setattr("amfv_datasets.scraping.cli._progress_documents", fake_progress) result = runner.invoke( @@ -199,13 +201,13 @@ def test_cli_run_accepts_all_documents(monkeypatch: pytest.MonkeyPatch) -> None: runner = CliRunner() def fake_scrape_documents( - source: ScraperSource, + source: str, *, documents: int | None, link_mode: LinkMode, url: str | None = None, ) -> ScrapeRun: - assert source is ScraperSource.ALL + assert source == "all" assert documents is None assert link_mode is LinkMode.KEEP assert url is None @@ -219,6 +221,22 @@ def fake_scrape_documents( assert json.loads(result.stdout.splitlines()[0])["external_id"] == "nice-ng1" +def test_cli_run_rejects_an_unregistered_source() -> None: + """An unknown --source names the sources that are registered.""" + runner = CliRunner() + + result = runner.invoke(app, ["--source", "nhs"]) + + assert result.exit_code != 0 + assert "'nhs'" in result.stderr + assert "all, nice" in result.stderr + + +def test_expand_source_runs_every_registered_scraper() -> None: + """The all source expands to the registry rather than a hand-written list.""" + assert _expand_source(ALL_SOURCES) == tuple(SCRAPERS) + + def _document() -> ScrapedDocument: return ScrapedDocument( source="nice", diff --git a/datasets/test/test_scraping_html.py b/datasets/test/test_scraping_html.py index d34a8fa3..69a0ffe2 100644 --- a/datasets/test/test_scraping_html.py +++ b/datasets/test/test_scraping_html.py @@ -68,3 +68,21 @@ def test_html_to_markdown_can_strip_links() -> None: html_text = '

Offer treatment.

' assert html_to_markdown(html_text, link_mode=LinkMode.STRIP) == "Offer treatment." + + +def test_html_to_markdown_absolutizes_images() -> None: + """Relative image sources are preserved as absolute markdown image URLs.""" + html_text = 'Treatment flowchart' + + assert html_to_markdown(html_text, base_url="https://example.org/guideline/") == ( + "![Treatment flowchart](https://example.org/images/flowchart.png)" + ) + + +def test_html_to_markdown_keeps_in_page_anchors_relative() -> None: + """A link to a heading in the same document is not rewritten to the base URL.""" + html_text = '

See recommendations.

' + + assert html_to_markdown(html_text, base_url="https://example.org/guideline/") == ( + "See [recommendations](#recommendations)." + )