Repository navigation
Conversation
github-actions
Bot
added
t-tooling
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v2 #2204 +/- ##
=====================================
Coverage ? 94.20%
=====================================
Files ? 185
Lines ? 13752
Branches ? 0
=====================================
Hits ? 12955
Misses ? 797
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Waiting for V2 as this might be breaking change and will inlcude removal of deprecated fields |
Pijukatel
changed the title
4b9f3e5 to
8af9ee3
Compare
de270b1 to
7ca479e
Compare
…r resurrection The `crawler_runtime` statistic no longer counts the downtime between the runs of a resumed crawler, and it accumulates correctly across any number of persist/resume cycles: - The first periodic statistics log reports exactly the runtime accumulated so far: zero for a fresh start, the previously persisted total for a resumed run. - The accumulated runtime is stored in the new `accumulated_runtime` field, persisted under `crawlerRuntimeMillis`. Every persist writes the live total, so a state persisted mid-run (migration, abort) carries the runtime up to that moment and a resumed run continues from the last checkpoint. - The timestamps of the previous run are cleared before the new run starts being measured, so they cannot leak the downtime into the runtime, and a backwards clock jump during a run cannot push the runtime below the accumulated total. BREAKING CHANGE: `StatisticsState.crawler_runtime_for_serialization` is removed; the `crawler_runtime` property is backed by the new `accumulated_runtime` field. `crawlerRuntimeMillis` is serialized as a number of milliseconds instead of an ISO 8601 duration string. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VMHwDx87jV4rnu5LkbpNxy
7ca479e to
fbab2a8
Compare
apify-factory
left a comment
apify-factory
left a comment
There was a problem hiding this comment.
Looks good, just one note.
Outside the diff:
src/crawlee/statistics/_models.py: Note:crawlerRuntimeMillisis serialized as the total (including the last run's segment) but validated back intoaccumulated_runtimealongside the unchangedcrawlerLastStartTimestamp/crawlerFinishedAt, soStatisticsState.model_validate(state.model_dump(by_alias=True)).crawler_runtimecounts the last run twice. OnlyStatistics.__aenter__clears the timestamps, so any consumer that parses a persisted statistics record withStatisticsStategets an inflated runtime.
🏭 Factory review — ℹ️ not auto-approved
This change was reviewed but cannot be auto-approved: a gate failed, so the review cannot end in an approval.
| check | result |
|---|---|
| Static gates | ❌ 13/16 — base-branch, max-lines, pr-title |
| Review findings | ❌ not clean |
| Reviewer escalation | ❌ Changes the persisted statistics format (crawlerRuntimeMillis type and meaning) and removes a public model field, a breaking public contract that downstream consumers like the Apify SDK may read. |
Juror 1 — claude-sonnet-5-5, adversarial |
⏭️ skipped — a gate failed |
Why it was not auto-approved
Failed gates:
base-branch: base is v2, required mastermax-lines: 217 changed lines (+194/-23), limit 200pr-title: breaking changes are never auto-approved
The comments above are the review itself; this section only explains the approval decision. Push a change and request @apify-factory again to re-run it, or ask a human to review.
Reviewed fc873682f · workflow run
That is not how the |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VMHwDx87jV4rnu5LkbpNxy
| if self.crawler_last_started_at is None: | ||
| return self.accumulated_runtime | ||
| finished_at = self.crawler_finished_at or datetime.now(UTC) | ||
| return self.accumulated_runtime + max(timedelta(), finished_at - self.crawler_last_started_at) |
There was a problem hiding this comment.
Important: A finished run is counted twice when the persisted state is loaded outside Statistics. crawlerRuntimeMillis already includes the last run, but crawlerLastStartTimestamp and crawlerFinishedAt are persisted next to it, so StatisticsState.model_validate(...) adds finished - last_started again. A 10s run reads back as 20s. If __aexit__ folds the run into accumulated_runtime (see the comment there), this property can ignore finished runs:
| if self.crawler_last_started_at is None: | |
| return self.accumulated_runtime | |
| finished_at = self.crawler_finished_at or datetime.now(UTC) | |
| return self.accumulated_runtime + max(timedelta(), finished_at - self.crawler_last_started_at) | |
| if self.crawler_last_started_at is None or self.crawler_finished_at is not None: | |
| return self.accumulated_runtime | |
| return self.accumulated_runtime + max(timedelta(), datetime.now(UTC) - self.crawler_last_started_at) |
✍️ Drafted by Claude Code
| # Stop logging; the finish timestamp freezes the measured run before the final persist. | ||
| await self._periodic_logger.stop() | ||
| self.state.crawler_finished_at = datetime.now(UTC) |
There was a problem hiding this comment.
Important: The other half of the double-count fix in _models.py:
| # Stop logging; the finish timestamp freezes the measured run before the final persist. | |
| await self._periodic_logger.stop() | |
| self.state.crawler_finished_at = datetime.now(UTC) | |
| # Stop logging and fold the finished run into the accumulated runtime before the final persist. | |
| await self._periodic_logger.stop() | |
| self.state.accumulated_runtime = self.state.crawler_runtime | |
| self.state.crawler_finished_at = datetime.now(UTC) |
✍️ Drafted by Claude Code
| def model_post_init(self, /, __context: Any) -> None: | ||
| self._runtime_offset = self.crawler_runtime or self._runtime_offset | ||
| accumulated_runtime: Annotated[timedelta_ms, Field(alias='crawlerRuntimeMillis')] = timedelta() | ||
| """The runtime accumulated by the previous runs of the crawler, excluding the run in progress.""" |
There was a problem hiding this comment.
Note: The serialized crawlerRuntimeMillis includes the run in progress, so a mid-run persist loaded with model_validate outside Statistics still counts the time since crawler_last_started_at twice. __aenter__ clears the timestamps, so the crawler itself is fine. I would leave the logic alone, but the docstring could mention that the persisted value differs.
✍️ Drafted by Claude Code
| async with Statistics.with_default_state(persistence_enabled=True, persist_state_key=key) as statistics: | ||
| pass | ||
|
|
||
| assert previous_runtime <= statistics.state.crawler_runtime < previous_runtime + timedelta(minutes=1) |
There was a problem hiding this comment.
Note: Two cases have no test: the double-count above, and restoring a runtime that v1 persisted as an ISO 8601 duration string.
| assert previous_runtime <= statistics.state.crawler_runtime < previous_runtime + timedelta(minutes=1) | |
| assert previous_runtime <= statistics.state.crawler_runtime < previous_runtime + timedelta(minutes=1) | |
| async def test_persisted_finished_state_validates_to_same_runtime() -> None: | |
| """A state persisted after a finished run reports the same runtime when validated back into the model.""" | |
| async with Statistics.with_default_state() as statistics: | |
| assert statistics.state.crawler_last_started_at | |
| statistics.state.crawler_last_started_at -= timedelta(seconds=10) | |
| serialized = statistics.state.model_dump(mode='json', by_alias=True) | |
| restored = StatisticsState.model_validate(serialized) | |
| assert abs(restored.crawler_runtime - statistics.state.crawler_runtime) < timedelta(milliseconds=1) | |
| async def test_runtime_restored_from_v1_iso_duration() -> None: | |
| """A runtime persisted by crawlee v1 as an ISO 8601 duration string is restored on resume.""" | |
| key = 'statistics_v1_runtime' | |
| previous_runtime = timedelta(seconds=10) | |
| kvs = await KeyValueStore.open() | |
| await kvs.set_value(key, {'crawlerRuntimeMillis': 'PT10S'}) | |
| async with Statistics.with_default_state(persistence_enabled=True, persist_state_key=key) as statistics: | |
| runtime = statistics.state.crawler_runtime | |
| assert previous_runtime <= runtime < previous_runtime + timedelta(minutes=1) |
✍️ Drafted by Claude Code
| @@ -17,6 +17,10 @@ We drop support for Python 3.10, which reached its end of life in October 2026. | |||
|
|
|||
| `StatisticsState.crawler_runtime` is computed from the crawler's start and finish timestamps. In v1, assigning to it only emitted a `DeprecationWarning` and the value was ignored. In v2, the property has no setter, so an assignment raises `AttributeError`. Remove any such assignments. | |||
There was a problem hiding this comment.
Note: With this PR, crawler_runtime no longer comes from the start and finish timestamps.
| `StatisticsState.crawler_runtime` is computed from the crawler's start and finish timestamps. In v1, assigning to it only emitted a `DeprecationWarning` and the value was ignored. In v2, the property has no setter, so an assignment raises `AttributeError`. Remove any such assignments. | |
| `StatisticsState.crawler_runtime` is computed from the runtime accumulated by previous runs and the start timestamp of the run in progress. In v1, assigning to it only emitted a `DeprecationWarning` and the value was ignored. In v2, the property has no setter, so an assignment raises `AttributeError`. Remove any such assignments. |
✍️ Drafted by Claude Code
| def _serialize_accumulated_runtime(self, _value: timedelta) -> float: | ||
| """Serialize the total runtime. Previously accumulated and current runtime.""" |
There was a problem hiding this comment.
Nit: round() returns an int, and the annotation feeds the serialization JSON schema. The docstring reads a bit awkwardly too.
| def _serialize_accumulated_runtime(self, _value: timedelta) -> float: | |
| """Serialize the total runtime. Previously accumulated and current runtime.""" | |
| def _serialize_accumulated_runtime(self, _value: timedelta) -> int: | |
| """Serialize the total runtime including the run in progress, so that every persist checkpoints it.""" |
✍️ Drafted by Claude Code
| from pydantic import ( | ||
| BaseModel, | ||
| ConfigDict, | ||
| Field, | ||
| PlainSerializer, | ||
| PlainValidator, | ||
| computed_field, | ||
| field_serializer, | ||
| ) |
There was a problem hiding this comment.
Nit: Fits on one line (116 chars).
| from pydantic import ( | |
| BaseModel, | |
| ConfigDict, | |
| Field, | |
| PlainSerializer, | |
| PlainValidator, | |
| computed_field, | |
| field_serializer, | |
| ) | |
| from pydantic import BaseModel, ConfigDict, Field, PlainSerializer, PlainValidator, computed_field, field_serializer |
✍️ Drafted by Claude Code
| # The restored timestamps belong to the previous run and must not measure this one: left in place, | ||
| # they would count the downtime between the runs (e.g. after a migration or resurrection) towards | ||
| # the runtime. Clear them, so that the initial periodic log reports exactly the runtime accumulated | ||
| # by the previous runs (zero for a fresh start). |
There was a problem hiding this comment.
Nit: Could be shorter.
| # The restored timestamps belong to the previous run and must not measure this one: left in place, | |
| # they would count the downtime between the runs (e.g. after a migration or resurrection) towards | |
| # the runtime. Clear them, so that the initial periodic log reports exactly the runtime accumulated | |
| # by the previous runs (zero for a fresh start). | |
| # The restored timestamps belong to the previous run. Clear them, so that the downtime since then (e.g. a | |
| # migration or resurrection) doesn't count towards the runtime. |
✍️ Drafted by Claude Code
|
|
||
|
|
||
| async def test_runtime_stops_growing_after_run_finishes() -> None: | ||
| """The runtime measured by a finished run stays frozen instead of following the clock.""" |
There was a problem hiding this comment.
Nit: Simpler wording.
| """The runtime measured by a finished run stays frozen instead of following the clock.""" | |
| """The runtime of a finished run stays fixed as time passes.""" |
✍️ Drafted by Claude Code
|
|
||
| def model_post_init(self, /, __context: Any) -> None: | ||
| self._runtime_offset = self.crawler_runtime or self._runtime_offset | ||
| accumulated_runtime: Annotated[timedelta_ms, Field(alias='crawlerRuntimeMillis')] = timedelta() |
There was a problem hiding this comment.
Do we really want to have the crawlerRuntimeMillis alias? Why? For serialization? Validation? Or both?
|
The title is 84 characters long. Could we use something more concise? E.g.:
|
crawler_runtimeincluding the downtime between the runs (e.g.38.01sfor a ~0.5 s run); later logs corrected themselves. Now the first log reports exactly the runtime accumulated so far (0 for a fresh run).StatisticsState.accumulated_runtimenow stores the runtime of the previous runs; the read-onlycrawler_runtimeproperty adds the run in progress, and every persist checkpoints the live total.crawler_runtime_for_serializationis removed, andcrawlerRuntimeMillisis persisted as a number of milliseconds instead of an ISO 8601 duration string. Documented indocs/upgrading/upgrading_to_v2.md.StatisticsState.crawler_runtimesetter #1567.