Skip to content

refactor: PersistentObject abstraction #1172

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

refactor: PersistentObject abstraction #1172

wants to merge 9 commits into from

Conversation

janbuchar
Copy link
Collaborator

@janbuchar janbuchar commented Apr 23, 2025 •
edited
Loading

Key Changes

  • Introduced a new PersistentObject helper class that:

    • Uses Pydantic models for state representation and validation
    • Handles state persistence to a KeyValueStore
    • Automatically registers/deregisters with event system for state persistence

  • Refactored existing components to use the new abstraction:

    • Converted SessionPool to use PersistentObject for state management
    • Converted Statistics to use PersistentObject for state management

Benefits

  • More consistent handling of state persistence across the library
  • Reduced code duplication for state management logic
  • Better separation of concerns between state data and persistence logic

@janbuchar janbuchar added the t-tooling Issues with this label are in the ownership of the tooling team. label Apr 23, 2025
@janbuchar janbuchar requested review from Pijukatel and vdusek April 23, 2025 13:54
@github-actions github-actions bot added this to the 113rd sprint - Tooling team milestone Apr 23, 2025
@github-actions github-actions bot added the tested Temporary label used only programatically for some analytics. label Apr 23, 2025
@janbuchar janbuchar force-pushed the persistent-object-abstraction branch from 1186ca2 to c1f920c Compare April 24, 2025 09:38
janbuchar
janbuchar commented Apr 24, 2025
View reviewed changes
src/crawlee/statistics/__init__.py
@@ -1,6 +1,6 @@
# ruff: noqa: A005

from ._models import FinalStatistics, StatisticsPersistedState, StatisticsState
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could revert removing this one for BC, but honestly, having it exported feels like a bug to me.

janbuchar
janbuchar commented Apr 24, 2025
View reviewed changes
tests/unit/sessions/test_models.py
@@ -78,33 +78,3 @@ def test_session_model(

# Check that max_age is correctly parsed into a timedelta object
assert session_direct.max_age == session_camel.max_age == session_snake.max_age == timedelta(minutes=30)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the new models, this didn't really test anything (except that assignment works..)

@janbuchar janbuchar marked this pull request as ready for review April 24, 2025 14:08
Pijukatel
Pijukatel reviewed Apr 25, 2025
View reviewed changes
src/crawlee/_utils/persistent_object.py
TStateModel = TypeVar('TStateModel', bound=BaseModel)


class PersistentObject(Generic[TStateModel]):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PersistentObject name creates impression of unconditional persistence, but PersistentObject(...persistence_enabled=False) is basically creating "NonPersitentObejct"

Same goes for usage in other classes that set persistence_enabled=False and then internally they use PersitentObject regardless.

Maybe it should have some name that hints about the persistence possibility. Maybe PersitableObject or something like that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. On the other hand, PersistableObject would suggest that you need to do something to actually persist it, at least for me. Brainstorm time!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude's suggestions kinda disappointed me... But I guess we could also work with words like "Checkpointing" or "Recoverable"

@janbuchar
Copy link
Collaborator Author

It would make a lot of sense to use the same abstraction for autosaved values in KeyValueStore

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

@Pijukatel Pijukatel Pijukatel left review comments

@vdusek vdusek Awaiting requested review from vdusek

Assignees

@janbuchar janbuchar

Labels
t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.
Projects
None yet
Milestone
113rd sprint - Tooling team
Development

Successfully merging this pull request may close these issues.

Extract state persistence into a mixin-style base class
2 participants
@janbuchar @Pijukatel