diff --git a/docs/upgrading/upgrading_to_v2.md b/docs/upgrading/upgrading_to_v2.md index 4bfa59ae51..52efafec25 100644 --- a/docs/upgrading/upgrading_to_v2.md +++ b/docs/upgrading/upgrading_to_v2.md @@ -95,3 +95,11 @@ The `Request.forefront` property is removed. Proxy tiers were the only code that The storage clients now write their JSON data with camelCase keys, the same as Crawlee JS. This covers the metadata, request and record metadata files of `FileSystemStorageClient`, the request and record metadata JSON stored by `RedisStorageClient`, and the request JSON stored by `SqlStorageClient`. For example, `item_count` in a dataset's `__metadata__.json` becomes `itemCount`. The storage metadata documents of `RedisStorageClient` keep their snake_case keys. Data written by v1 still loads, so an existing storage directory, Redis instance or database keeps working. Any tooling that reads these files directly needs to handle the camelCase keys. + +## Crawler `id` is a string + +The `id` argument of `BasicCrawler` and its subclasses is now a `str`, so pass a string wherever you passed an integer. `StatisticsState.stats_id` and `EventCrawlerStatusData.crawler_id` are strings too, and `crawler_id` holds the crawler's `id`. In v1, it held the Python object ID of the crawler. + +The statistics a crawler creates by default now use the crawler's `id`. Crawlers that share an `id` therefore share both the state returned by `use_state` and the persisted statistics. In v1, they shared only the state. + +A crawler without an `id` gets its creation index, such as `'0'`, and crawlers with an explicit `id` now count towards that index too. If you resume a v1 run with `purge_on_start=False` and mix both kinds of crawlers, a generated `id` can differ from the one v1 assigned, and the crawler won't find the state that `use_state` stored under the old one. diff --git a/src/crawlee/crawlers/_basic/_basic_crawler.py b/src/crawlee/crawlers/_basic/_basic_crawler.py index f091c1dff0..af2619384f 100644 --- a/src/crawlee/crawlers/_basic/_basic_crawler.py +++ b/src/crawlee/crawlers/_basic/_basic_crawler.py @@ -218,9 +218,9 @@ class _BasicCrawlerOptions(TypedDict): """Allows overriding the default status message. The default status message is provided in the parameters. Returning `None` suppresses the status message.""" - id: NotRequired[int] - """Identifier used for crawler state tracking. Use the same id across multiple crawlers to share state between - them.""" + id: NotRequired[str] + """A unique identifier for the crawler instance. It isolates the `use_state` state and the default statistics from + other crawlers. Without it, the ID follows the order in which crawlers are created.""" class _BasicCrawlerOptionsGeneric(TypedDict, Generic[TCrawlingContext, TStatisticsState]): @@ -307,7 +307,7 @@ def __init__( status_message_logging_interval: timedelta = timedelta(seconds=10), status_message_callback: Callable[[StatisticsState, StatisticsState | None, str], Awaitable[str | None]] | None = None, - id: int | None = None, + id: str | None = None, _context_pipeline: ContextPipeline[TCrawlingContext] | None = None, _additional_context_managers: Sequence[AbstractAsyncContextManager] | None = None, _logger: logging.Logger | None = None, @@ -362,8 +362,8 @@ def __init__( status_message_logging_interval: Interval for logging the crawler status messages. status_message_callback: Allows overriding the default status message. The default status message is provided in the parameters. Returning `None` suppresses the status message. - id: Identifier used for crawler state tracking. Use the same id across multiple crawlers to share state - between them. + id: A unique identifier for the crawler instance. It isolates the `use_state` state and the default + statistics from other crawlers. Without it, the ID follows the order in which crawlers are created. _context_pipeline: Enables extending the request lifecycle and modifying the crawling context. Intended for use by subclasses rather than direct instantiation of `BasicCrawler`. _additional_context_managers: Additional context managers used throughout the crawler lifecycle. @@ -371,11 +371,8 @@ def __init__( _logger: A logger instance, typically provided by a subclass, for consistent logging labels. Intended for use by subclasses rather than direct instantiation of `BasicCrawler`. """ - if id is None: - self._id = BasicCrawler.__next_id - BasicCrawler.__next_id += 1 - else: - self._id = id + self._id = str(BasicCrawler.__next_id) if id is None else id + BasicCrawler.__next_id += 1 implicit_event_manager_with_explicit_config = False if not configuration: @@ -485,6 +482,7 @@ async def persist_state_factory() -> KeyValueStore: statistics_log_format=self._statistics_log_format, log_message='Current request statistics:', persist_state_kvs_factory=persist_state_factory, + id=self._id, ), ) @@ -1883,7 +1881,7 @@ async def _crawler_state_task(self) -> None: self._log_status_message(message, level='INFO') event_manager.emit( - event=Event.CRAWLER_STATUS, event_data=EventCrawlerStatusData(message=message, crawler_id=id(self)) + event=Event.CRAWLER_STATUS, event_data=EventCrawlerStatusData(message=message, crawler_id=self._id) ) self._previous_crawler_state = current_state diff --git a/src/crawlee/events/_types.py b/src/crawlee/events/_types.py index 383b228958..d26887076c 100644 --- a/src/crawlee/events/_types.py +++ b/src/crawlee/events/_types.py @@ -90,7 +90,7 @@ class EventCrawlerStatusData(BaseModel): message: str """A message describing the current status of the crawler.""" - crawler_id: int + crawler_id: str """The ID of the crawler that emitted the event.""" diff --git a/src/crawlee/statistics/_models.py b/src/crawlee/statistics/_models.py index ede4037859..1db25cd5ab 100644 --- a/src/crawlee/statistics/_models.py +++ b/src/crawlee/statistics/_models.py @@ -61,7 +61,8 @@ class StatisticsState(BaseModel): model_config = ConfigDict( alias_generator=to_camel, validate_by_name=True, validate_by_alias=True, ser_json_inf_nan='constants' ) - stats_id: int | None = None + # Statistics persisted by crawlee v1 store the ID as a number. + stats_id: Annotated[str | None, Field(coerce_numbers_to_str=True)] = None requests_finished: int = 0 requests_failed: int = 0 diff --git a/src/crawlee/statistics/_statistics.py b/src/crawlee/statistics/_statistics.py index 4dccaa8ba7..0e39aa3557 100644 --- a/src/crawlee/statistics/_statistics.py +++ b/src/crawlee/statistics/_statistics.py @@ -85,8 +85,9 @@ def __init__( state_model: type[TStatisticsState], statistics_log_format: Literal['table', 'inline'] = 'table', save_error_snapshots: bool = False, + id: str | None = None, ) -> None: - self._id = Statistics.__next_id + self._id = str(Statistics.__next_id) if id is None else id Statistics.__next_id += 1 self.error_tracker = ErrorTracker( @@ -123,6 +124,7 @@ def replace_state_model(self, state_model: type[TNewStatisticsState]) -> Statist log_message=self._log_message, periodic_message_logger=self._periodic_message_logger, state_model=state_model, + id=self._id, ) new_statistics._periodic_logger = self._periodic_logger # Accessing private member to create copy like-object. return new_statistics @@ -139,6 +141,7 @@ def with_default_state( log_interval: timedelta = timedelta(minutes=1), statistics_log_format: Literal['table', 'inline'] = 'table', save_error_snapshots: bool = False, + id: str | None = None, ) -> Statistics[StatisticsState]: """Initialize a new instance with default state model `StatisticsState`.""" return Statistics[StatisticsState]( @@ -152,6 +155,7 @@ def with_default_state( state_model=StatisticsState, statistics_log_format=statistics_log_format, save_error_snapshots=save_error_snapshots, + id=id, ) @property diff --git a/tests/unit/_statistics/test_persistence.py b/tests/unit/_statistics/test_persistence.py index 3c6e06d02d..3ee559e40c 100644 --- a/tests/unit/_statistics/test_persistence.py +++ b/tests/unit/_statistics/test_persistence.py @@ -1,6 +1,9 @@ from __future__ import annotations -from crawlee.statistics import Statistics +import pytest + +from crawlee.statistics import Statistics, StatisticsState +from crawlee.storages import KeyValueStore async def test_basic_persistence() -> None: @@ -13,3 +16,48 @@ async def test_basic_persistence() -> None: pass assert statistics.state.requests_failed == 42 + + +@pytest.mark.parametrize( + ('second_id', 'expected_requests_failed'), + [ + pytest.param('a', 42, id='same id'), + pytest.param('b', 0, id='different id'), + ], +) +async def test_persistence_by_id(second_id: str, expected_requests_failed: int) -> None: + """Statistics with the same ID share the persisted state, and statistics with different IDs don't.""" + async with Statistics.with_default_state(persistence_enabled=True, id='a') as statistics: + statistics.state.requests_failed = 42 + + async with Statistics.with_default_state(persistence_enabled=True, id=second_id) as statistics: + pass + + assert statistics.state.requests_failed == expected_requests_failed + + +async def test_persistence_loads_numeric_id() -> None: + """A persisted state whose `statsId` is a number loads with a string ID.""" + key = 'statistics_foo' + kvs = await KeyValueStore.open() + await kvs.set_value(key, {'statsId': 0, 'requestsFailed': 42}) + + async with Statistics.with_default_state(persistence_enabled=True, persist_state_key=key) as statistics: + pass + + assert statistics.state.stats_id == '0' + assert statistics.state.requests_failed == 42 + + +def test_id_defaults_to_counter() -> None: + """Statistics without an ID take their creation index, which statistics with an explicit ID advance too.""" + assert Statistics.with_default_state()._id == '0' + assert Statistics.with_default_state(id='custom')._id == 'custom' + assert Statistics.with_default_state()._id == '2' + + +def test_replace_state_model_keeps_id() -> None: + """`replace_state_model` keeps the ID of the original statistics.""" + statistics = Statistics.with_default_state(id='custom') + + assert statistics.replace_state_model(StatisticsState)._id == 'custom' diff --git a/tests/unit/crawlers/_basic/test_basic_crawler.py b/tests/unit/crawlers/_basic/test_basic_crawler.py index cb80b41837..e9ada3a9f7 100644 --- a/tests/unit/crawlers/_basic/test_basic_crawler.py +++ b/tests/unit/crawlers/_basic/test_basic_crawler.py @@ -39,7 +39,7 @@ from crawlee.http_clients import HttpClient from crawlee.request_loaders import RequestList, RequestManagerTandem, ThrottlingRequestManager from crawlee.sessions import Session, SessionPool -from crawlee.statistics import FinalStatistics, StatisticsState +from crawlee.statistics import FinalStatistics, Statistics, StatisticsState from crawlee.storage_clients import FileSystemStorageClient, MemoryStorageClient from crawlee.storages import Dataset, KeyValueStore, RequestQueue from tests.unit.utils import open_throttler_held_until_reclaim, poll_until_condition @@ -1187,14 +1187,14 @@ async def handler(context: BasicCrawlingContext) -> None: assert isinstance(state['urls'], list) state['urls'].append(context.request.url) - crawler_1 = BasicCrawler(id=0, request_handler=handler) - crawler_2 = BasicCrawler(id=0, request_handler=handler) + crawler_1 = BasicCrawler(id='0', request_handler=handler) + crawler_2 = BasicCrawler(id='0', request_handler=handler) await crawler_1.run(['https://a.com']) await crawler_2.run(['https://b.com']) kvs = await KeyValueStore.open() - assert crawler_1._id == crawler_2._id == 0 + assert crawler_1._id == crawler_2._id == '0' assert await kvs.get_value(f'{BasicCrawler._CRAWLEE_STATE_KEY}_{crawler_1._id}') == { 'urls': ['https://a.com', 'https://b.com'] } @@ -1204,8 +1204,8 @@ async def test_crawlers_share_stats() -> None: async def handler(context: BasicCrawlingContext) -> None: await context.use_state({'urls': []}) - crawler_1 = BasicCrawler(id=0, request_handler=handler) - crawler_2 = BasicCrawler(id=0, request_handler=handler, statistics=crawler_1.statistics) + crawler_1 = BasicCrawler(id='0', request_handler=handler) + crawler_2 = BasicCrawler(id='0', request_handler=handler, statistics=crawler_1.statistics) result1 = await crawler_1.run(['https://a.com']) result2 = await crawler_2.run(['https://b.com']) @@ -1232,6 +1232,43 @@ async def handler(context: BasicCrawlingContext) -> None: assert await kvs.get_value(f'{BasicCrawler._CRAWLEE_STATE_KEY}_1') == {'urls': ['https://b.com']} +@pytest.mark.parametrize( + ('crawler_id', 'expected_id'), + [ + pytest.param(None, '0', id='generated id'), + pytest.param('my-crawler', 'my-crawler', id='explicit id'), + ], +) +async def test_default_statistics_use_crawler_id(crawler_id: str | None, expected_id: str) -> None: + """The default statistics take the crawler ID and persist under it.""" + # Advance the `Statistics` counter, so an ID the statistics generate themselves can't match the crawler ID. + Statistics.with_default_state() + crawler = BasicCrawler(request_handler=AsyncMock(), id=crawler_id) + + await crawler.run(['https://a.placeholder.com']) + + assert crawler._id == expected_id + assert crawler.statistics.state.stats_id == expected_id + kvs = await crawler.get_key_value_store() + persisted_state = await kvs.get_value(f'__CRAWLER_STATISTICS_{expected_id}') + assert persisted_state['statsId'] == expected_id + + +def test_generated_id_follows_creation_order() -> None: + """A crawler without an ID takes its creation index, which crawlers with an explicit ID advance too.""" + assert BasicCrawler()._id == '0' + assert BasicCrawler(id='custom')._id == 'custom' + assert BasicCrawler()._id == '2' + + +def test_default_statistics_key_differs_from_later_statistics() -> None: + """Statistics created after a crawler with default statistics don't persist under the crawler's key.""" + crawler = BasicCrawler() + statistics = Statistics.with_default_state(persistence_enabled=True) + + assert statistics._state._persist_state_key != crawler.statistics._state._persist_state_key + + async def test_context_handlers_use_state(key_value_store: KeyValueStore) -> None: state_in_handler_one: dict[str, JsonSerializable] = {} state_in_handler_two: dict[str, JsonSerializable] = {} @@ -2162,13 +2199,14 @@ def listener(event_data: EventCrawlerStatusData) -> None: event_manager.on(event=Event.CRAWLER_STATUS, listener=listener) - crawler = BasicCrawler(request_handler=AsyncMock()) + crawler = BasicCrawler(request_handler=AsyncMock(), id='my-crawler') await crawler.run(['https://a.placeholder.com']) event_manager.off(event=Event.CRAWLER_STATUS, listener=listener) assert status_message_listener.called + assert status_message_listener.call_args.args[0].crawler_id == 'my-crawler' async def test_status_message_reports_failed_request_count() -> None: @@ -2283,7 +2321,7 @@ async def handler(context: BasicCrawlingContext) -> None: assert await unrelated_rq.fetch_next_request() == unrelated_request -async def _run_crawler(crawler_id: int | None, requests: list[str], storage_dir: str) -> StatisticsState: +async def _run_crawler(crawler_id: str | None, requests: list[str], storage_dir: str) -> StatisticsState: """Run crawler and return its statistics state. Must be defined like this to be pickable for ProcessPoolExecutor.""" @@ -2314,7 +2352,7 @@ async def request_handler(context: BasicCrawlingContext) -> None: @dataclass class _CrawlerInput: requests: list[str] - id: int | None = None + id: str | None = None def _process_run_crawlers(crawler_inputs: list[_CrawlerInput], storage_dir: str) -> list[StatisticsState]: