-
Notifications
You must be signed in to change notification settings - Fork 819
FEAT: Add remove_seeds_from_memory_async to memory #2243
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2070,7 +2070,7 @@ def cleanup(self) -> None: | |
| # Ensure cleanup happens even if the object is garbage collected before process exits | ||
| weakref.finalize(self, self.dispose_engine) | ||
|
|
||
| def get_seeds( | ||
| def _build_seed_filter_conditions( | ||
| self, | ||
| *, | ||
| value: str | None = None, | ||
|
|
@@ -2087,13 +2087,17 @@ def get_seeds( | |
| parameters: Sequence[str] | None = None, | ||
| metadata: dict[str, str | int] | None = None, | ||
| prompt_group_ids: Sequence[uuid.UUID] | None = None, | ||
| ) -> Sequence[Seed]: | ||
| ) -> list[Any]: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make this type stricter than list[Any]? The file already uses list[ColumnElement[bool]] for condition builders. Using Any here removes useful checking from both query and deletion paths. |
||
| """ | ||
| Retrieve a list of seed prompts based on the specified filters. | ||
| Build SQLAlchemy filter conditions shared by seed query and removal methods. | ||
|
|
||
| This centralizes the filter-building logic so that get_seeds and | ||
| remove_seeds_from_memory_async stay in sync and cannot drift. | ||
|
|
||
| Args: | ||
| value (str): The value to match by substring. If None, all values are returned. | ||
|
|
||
| value_sha256 (str): The SHA256 hash of the value to match. If None, all values are returned. | ||
| value_sha256 (Sequence[str] | None): A list of SHA256 hashes of values to match. | ||
| If None, all values are returned. | ||
| dataset_name (str): The dataset name to match exactly. If None, all dataset names are considered. | ||
| dataset_name_pattern (str): A pattern to match dataset names using SQL LIKE syntax. | ||
| Supports wildcards: % (any characters) and _ (single character). | ||
|
|
@@ -2118,9 +2122,9 @@ def get_seeds( | |
| prompt_group_ids (Sequence[uuid.UUID]): A list of prompt group IDs to filter by. | ||
|
|
||
| Returns: | ||
| Sequence[SeedPrompt]: A list of prompts matching the criteria. | ||
| list[Any]: A list of SQLAlchemy filter conditions. | ||
| """ | ||
| conditions = [] | ||
| conditions: list[Any] = [] | ||
|
|
||
| # Apply filters for non-list fields | ||
| if value: | ||
|
|
@@ -2157,6 +2161,77 @@ def get_seeds( | |
| if metadata: | ||
| conditions.append(self._get_seed_metadata_conditions(metadata=metadata)) | ||
|
|
||
| return conditions | ||
|
|
||
| def get_seeds( | ||
| self, | ||
| *, | ||
| value: str | None = None, | ||
| value_sha256: Sequence[str] | None = None, | ||
| dataset_name: str | None = None, | ||
| dataset_name_pattern: str | None = None, | ||
| data_types: Sequence[str] | None = None, | ||
| harm_categories: Sequence[str] | None = None, | ||
| added_by: str | None = None, | ||
| authors: Sequence[str] | None = None, | ||
| groups: Sequence[str] | None = None, | ||
| source: str | None = None, | ||
| seed_type: SeedType | None = None, | ||
| parameters: Sequence[str] | None = None, | ||
| metadata: dict[str, str | int] | None = None, | ||
| prompt_group_ids: Sequence[uuid.UUID] | None = None, | ||
| ) -> Sequence[Seed]: | ||
| """ | ||
| Retrieve a list of seed prompts based on the specified filters. | ||
|
|
||
| Args: | ||
| value (str): The value to match by substring. If None, all values are returned. | ||
| value_sha256 (Sequence[str] | None): A list of SHA256 hashes of values to match. | ||
| If None, all values are returned. | ||
| dataset_name (str): The dataset name to match exactly. If None, all dataset names are considered. | ||
| dataset_name_pattern (str): A pattern to match dataset names using SQL LIKE syntax. | ||
| Supports wildcards: % (any characters) and _ (single character). | ||
| Examples: "harm%" matches names starting with "harm", "%test%" matches names containing "test". | ||
| If both dataset_name and dataset_name_pattern are provided, dataset_name takes precedence. | ||
| data_types (Sequence[str] | None): List of data types to filter seed prompts by | ||
| (e.g., text, image_path). | ||
| harm_categories (Sequence[str]): A list of harm categories to filter by. If None, | ||
| all harm categories are considered. | ||
| Specifying multiple harm categories returns only prompts that are marked with all harm categories. | ||
| added_by (str): The user who added the prompts. | ||
| authors (Sequence[str]): A list of authors to filter by. | ||
| Note that this filters by substring, so a query for "Adam Jones" may not return results if the record | ||
| is "A. Jones", "Jones, Adam", etc. If None, all authors are considered. | ||
| groups (Sequence[str]): A list of groups to filter by. If None, all groups are considered. | ||
| source (str): The source to filter by. If None, all sources are considered. | ||
| seed_type (SeedType): The type of seed to filter by ("prompt", "objective", or | ||
| "simulated_conversation"). | ||
| parameters (Sequence[str]): A list of parameters to filter by. Specifying parameters effectively returns | ||
| prompt templates instead of prompts. | ||
| metadata (dict[str, str | int]): A free-form dictionary for tagging prompts with custom metadata. | ||
| prompt_group_ids (Sequence[uuid.UUID]): A list of prompt group IDs to filter by. | ||
|
|
||
| Returns: | ||
| Sequence[Seed]: A list of seeds (e.g., SeedPrompt, SeedObjective, SeedSimulatedConversation) | ||
| matching the criteria. | ||
| """ | ||
| conditions = self._build_seed_filter_conditions( | ||
| value=value, | ||
| value_sha256=value_sha256, | ||
| dataset_name=dataset_name, | ||
| dataset_name_pattern=dataset_name_pattern, | ||
| data_types=data_types, | ||
| harm_categories=harm_categories, | ||
| added_by=added_by, | ||
| authors=authors, | ||
| groups=groups, | ||
| source=source, | ||
| seed_type=seed_type, | ||
| parameters=parameters, | ||
| metadata=metadata, | ||
| prompt_group_ids=prompt_group_ids, | ||
| ) | ||
|
|
||
| try: | ||
| memory_entries: Sequence[SeedEntry] = self._query_entries( | ||
| SeedEntry, | ||
|
|
@@ -2167,6 +2242,101 @@ def get_seeds( | |
| logger.exception(f"Failed to retrieve prompts with dataset name {dataset_name} with error {e}") | ||
| raise | ||
|
|
||
| async def remove_seeds_from_memory_async( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why async ? |
||
| self, | ||
| *, | ||
| value: str | None = None, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmmm i'm kinda concerned about using value for deletion. It's much safer when we're just getting a seed but if a user inputs "the" for example or a commonly used word the substring matching would delete a lot of seeds. |
||
| value_sha256: Sequence[str] | None = None, | ||
| dataset_name: str | None = None, | ||
| dataset_name_pattern: str | None = None, | ||
| data_types: Sequence[str] | None = None, | ||
| harm_categories: Sequence[str] | None = None, | ||
| added_by: str | None = None, | ||
| authors: Sequence[str] | None = None, | ||
| groups: Sequence[str] | None = None, | ||
| source: str | None = None, | ||
| seed_type: SeedType | None = None, | ||
| parameters: Sequence[str] | None = None, | ||
| metadata: dict[str, str | int] | None = None, | ||
| prompt_group_ids: Sequence[uuid.UUID] | None = None, | ||
| ) -> int: | ||
| """ | ||
| Remove seeds matching the specified filters from memory. | ||
|
|
||
| Accepts the same filter parameters as get_seeds. It is recommended to call get_seeds with the same | ||
| filters first to preview which seeds will be removed. At least one filter must be provided to | ||
| prevent accidental deletion of all seeds. The deletion runs in a single transaction, so it works | ||
| consistently across DuckDB, SQLite, and Azure SQL. | ||
|
Comment on lines
+2268
to
+2269
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DuckDB is no longer a supported memory backend. This should just say SQLite and Azure SQL. |
||
|
|
||
| Args: | ||
| value (str): The value to match by substring. If None, all values are considered. | ||
| value_sha256 (Sequence[str] | None): A list of SHA256 hashes of values to match. | ||
| If None, all values are considered. | ||
| dataset_name (str): The dataset name to match exactly. If None, all dataset names are considered. | ||
| dataset_name_pattern (str): A pattern to match dataset names using SQL LIKE syntax. | ||
| Supports wildcards: % (any characters) and _ (single character). | ||
| Examples: "harm%" matches names starting with "harm", "%test%" matches names containing "test". | ||
| If both dataset_name and dataset_name_pattern are provided, dataset_name takes precedence. | ||
| data_types (Sequence[str] | None): List of data types to filter seed prompts by | ||
| (e.g., text, image_path). | ||
| harm_categories (Sequence[str]): A list of harm categories to filter by. If None, | ||
| all harm categories are considered. | ||
| Specifying multiple harm categories matches only prompts that are marked with all harm categories. | ||
| added_by (str): The user who added the prompts. | ||
| authors (Sequence[str]): A list of authors to filter by. | ||
| Note that this filters by substring, so a query for "Adam Jones" may not return results if the record | ||
| is "A. Jones", "Jones, Adam", etc. If None, all authors are considered. | ||
| groups (Sequence[str]): A list of groups to filter by. If None, all groups are considered. | ||
| source (str): The source to filter by. If None, all sources are considered. | ||
| seed_type (SeedType): The type of seed to filter by ("prompt", "objective", or | ||
| "simulated_conversation"). | ||
| parameters (Sequence[str]): A list of parameters to filter by. Specifying parameters effectively targets | ||
| prompt templates instead of prompts. | ||
| metadata (dict[str, str | int]): A free-form dictionary for tagging prompts with custom metadata. | ||
| prompt_group_ids (Sequence[uuid.UUID]): A list of prompt group IDs to filter by. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so prompt_group_ids would remove entire groups ? I'm wondering if we should have a separate remove_seed_groups_from_memory (and similarly for datasets) |
||
|
|
||
| Returns: | ||
| int: The number of seeds removed. | ||
|
|
||
| Raises: | ||
| ValueError: If no filters are provided. | ||
| """ | ||
| conditions = self._build_seed_filter_conditions( | ||
| value=value, | ||
| value_sha256=value_sha256, | ||
| dataset_name=dataset_name, | ||
| dataset_name_pattern=dataset_name_pattern, | ||
| data_types=data_types, | ||
| harm_categories=harm_categories, | ||
| added_by=added_by, | ||
| authors=authors, | ||
| groups=groups, | ||
| source=source, | ||
| seed_type=seed_type, | ||
| parameters=parameters, | ||
| metadata=metadata, | ||
| prompt_group_ids=prompt_group_ids, | ||
| ) | ||
|
|
||
| # Guard against accidental "delete all": require at least one filter. | ||
| if not conditions: | ||
| raise ValueError( | ||
| "At least one filter parameter must be provided to remove_seeds_from_memory_async. " | ||
| "Calling without filters would delete all seeds." | ||
| ) | ||
|
|
||
| # Delete matching rows in a single transaction so it is atomic across backends. | ||
| with closing(self.get_session()) as session: | ||
| try: | ||
| query = session.query(SeedEntry).filter(and_(*conditions)) | ||
| count = query.delete(synchronize_session=False) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are we doing to file-backed image, audio, and video seeds? This removes the database row but leaves the serialized file behind. We should either clean up owned artifacts or clearly document that this only removes database records. |
||
| session.commit() | ||
| return count | ||
| except SQLAlchemyError as e: | ||
| session.rollback() | ||
| logger.exception(f"Failed to remove seeds from memory: {e}") | ||
| raise | ||
|
Comment on lines
+2328
to
+2338
|
||
|
|
||
| def _add_list_conditions( | ||
| self, field: InstrumentedAttribute[Any], conditions: list[Any], values: Sequence[str] | None = None | ||
| ) -> None: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just noting that there are some possible cases where deleting will cause unintended consequences:
these are just some examples and for the most part can be classified as user errors but might be something good to document