-
Notifications
You must be signed in to change notification settings - Fork 3k
COG-6341 feat: Add --memory-only flag to cognee-cli forget #4697
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: dev
Are you sure you want to change the base?
Changes from 3 commits
0e4c13a
d5fd682
14c7e7b
a2f6304
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 |
|---|---|---|
|
|
@@ -17,7 +17,9 @@ class ForgetCommand(SupportsCliCommand): | |
| Remove data from the knowledge graph. | ||
|
|
||
| Use --everything (alias --all) to delete all user data, --dataset/--dataset-id | ||
| to delete a dataset, or dataset + --data-id to delete a single item. | ||
| to delete a dataset, or dataset + --data-id to delete a single item. Add | ||
| --memory-only to clear graph/vector memory while keeping raw files and data | ||
| records, so the dataset (or item) can be re-cognified later. | ||
| """ | ||
|
|
||
| def configure_parser(self, parser: argparse.ArgumentParser) -> None: | ||
|
|
@@ -37,6 +39,16 @@ def configure_parser(self, parser: argparse.ArgumentParser) -> None: | |
| default=False, | ||
| help="Delete all datasets and data", | ||
| ) | ||
| parser.add_argument( | ||
| "--memory-only", | ||
| action="store_true", | ||
| default=False, | ||
| help=( | ||
| "Delete only graph/vector memory (requires --dataset or --dataset-id); " | ||
| "raw files and data records are preserved so the dataset can be " | ||
| "re-cognified with different settings" | ||
| ), | ||
| ) | ||
|
|
||
| def execute(self, args: argparse.Namespace) -> None: | ||
| try: | ||
|
|
@@ -56,13 +68,22 @@ def execute(self, args: argparse.Namespace) -> 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. [minor] Error message inconsistency. This says |
||
| return | ||
|
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. minor — Same validation gap as in api_dispatch.py: |
||
|
|
||
| if args.everything and args.memory_only: | ||
| fmt.error( | ||
| "--memory-only has no effect with --everything: everything deletes all " | ||
| "datasets and data outright. Specify --dataset or --dataset-id with " | ||
| "--memory-only instead." | ||
| ) | ||
| return | ||
|
|
||
| async def run_forget(): | ||
| try: | ||
| return await cognee.forget( | ||
| data_id=data_id, | ||
| dataset=dataset, | ||
| dataset_id=dataset_id, | ||
| everything=args.everything, | ||
| memory_only=args.memory_only, | ||
| ) | ||
| except Exception as e: | ||
| raise CliCommandInnerException(f"Failed to forget: {str(e)}") from e | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -177,3 +177,102 @@ def test_no_user_id_no_header(self, MockClient): | |
| call_kwargs = MockClient.call_args | ||
| headers = call_kwargs.kwargs.get("headers") or call_kwargs[1].get("headers", {}) | ||
| assert "X-User-Id" not in headers | ||
|
|
||
|
|
||
| class TestForgetDispatch: | ||
| """Finding 8 (COG-6335 review): --memory-only must reach the API client, | ||
| and a --dataset value must reach it too (args.dataset, not the | ||
| never-set args.dataset_name the dispatcher used to read).""" | ||
|
|
||
| @patch("cognee.cli.api_dispatch.CogneeApiClient") | ||
| def test_memory_only_and_dataset_forwarded_to_client(self, MockClient): | ||
| mock_instance = MagicMock() | ||
| mock_instance.forget.return_value = { | ||
| "status": "success", | ||
| "dataset_id": "ds-id", | ||
| "data_records_reset": 0, | ||
| } | ||
| MockClient.return_value.__enter__ = MagicMock(return_value=mock_instance) | ||
| MockClient.return_value.__exit__ = MagicMock(return_value=False) | ||
|
|
||
| args = argparse.Namespace( | ||
| api_url="http://localhost:8000", | ||
| command="forget", | ||
| user_id=None, | ||
| dataset="my_dataset", | ||
| dataset_id=None, | ||
| data_id=None, | ||
| everything=False, | ||
| memory_only=True, | ||
| ) | ||
| dispatch(args) | ||
|
|
||
| mock_instance.forget.assert_called_once_with( | ||
| dataset="my_dataset", | ||
| dataset_id=None, | ||
| data_id=None, | ||
| everything=False, | ||
| memory_only=True, | ||
| ) | ||
|
|
||
| @patch("cognee.cli.api_dispatch.CogneeApiClient") | ||
| def test_everything_with_memory_only_does_not_call_client(self, MockClient): | ||
| """--memory-only has no effect with --everything (which deletes | ||
| outright) -- must error instead of silently doing a full wipe.""" | ||
| mock_instance = MagicMock() | ||
| MockClient.return_value.__enter__ = MagicMock(return_value=mock_instance) | ||
| MockClient.return_value.__exit__ = MagicMock(return_value=False) | ||
|
|
||
| args = argparse.Namespace( | ||
| api_url="http://localhost:8000", | ||
| command="forget", | ||
| user_id=None, | ||
| dataset=None, | ||
| dataset_id=None, | ||
| data_id=None, | ||
| everything=True, | ||
| memory_only=True, | ||
| ) | ||
| dispatch(args) | ||
|
|
||
| mock_instance.forget.assert_not_called() | ||
|
|
||
| @patch("cognee.cli.api_dispatch.CogneeApiClient") | ||
| def test_dataset_and_dataset_id_both_set_does_not_call_client(self, MockClient): | ||
| mock_instance = MagicMock() | ||
| MockClient.return_value.__enter__ = MagicMock(return_value=mock_instance) | ||
| MockClient.return_value.__exit__ = MagicMock(return_value=False) | ||
|
|
||
| args = argparse.Namespace( | ||
| api_url="http://localhost:8000", | ||
| command="forget", | ||
| user_id=None, | ||
| dataset="my_dataset", | ||
| dataset_id="11111111-1111-1111-1111-111111111111", | ||
| data_id=None, | ||
| everything=False, | ||
| memory_only=False, | ||
| ) | ||
| dispatch(args) | ||
|
|
||
| mock_instance.forget.assert_not_called() | ||
|
|
||
| @patch("cognee.cli.api_dispatch.CogneeApiClient") | ||
| def test_missing_forget_target_does_not_call_client(self, MockClient): | ||
| mock_instance = MagicMock() | ||
| MockClient.return_value.__enter__ = MagicMock(return_value=mock_instance) | ||
| MockClient.return_value.__exit__ = MagicMock(return_value=False) | ||
|
|
||
| args = argparse.Namespace( | ||
| api_url="http://localhost:8000", | ||
| command="forget", | ||
| user_id=None, | ||
| dataset=None, | ||
| dataset_id=None, | ||
| data_id=None, | ||
| everything=False, | ||
| memory_only=False, | ||
| ) | ||
| dispatch(args) | ||
|
|
||
|
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. [blocker] Missing test for the main bug fix — The PR title says "Require dataset when --data-id is used" and the commit message explains that Add a test: @patch("cognee.cli.api_dispatch.CogneeApiClient")
def test_data_id_without_dataset_does_not_call_client(self, MockClient):
"""--data-id alone must error; requires --dataset or --dataset-id."""
mock_instance = MagicMock()
MockClient.return_value.__enter__ = MagicMock(return_value=mock_instance)
MockClient.return_value.__exit__ = MagicMock(return_value=False)
args = argparse.Namespace(
api_url="http://localhost:8000",
command="forget",
user_id=None,
dataset=None,
dataset_id=None,
data_id="11111111-1111-1111-1111-111111111111",
everything=False,
memory_only=False,
)
dispatch(args)
mock_instance.forget.assert_not_called() |
||
| mock_instance.forget.assert_not_called() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ | |
| from cognee.cli.commands.recall_command import RecallCommand | ||
| from cognee.cli.commands.cognify_command import CognifyCommand | ||
| from cognee.cli.commands.delete_command import DeleteCommand | ||
| from cognee.cli.commands.forget_command import ForgetCommand | ||
| from cognee.cli.commands.config_command import ConfigCommand | ||
| from cognee.cli.exceptions import CliCommandException | ||
| from cognee.modules.data.methods.get_deletion_counts import DeletionCountsPreview | ||
|
|
@@ -616,6 +617,98 @@ def test_execute_with_exception(self, mock_asyncio_run): | |
| command.execute(args) | ||
|
|
||
|
|
||
| class TestForgetCommand: | ||
| """Test the ForgetCommand class""" | ||
|
|
||
| def test_command_properties(self): | ||
| command = ForgetCommand() | ||
| assert command.command_string == "forget" | ||
| assert "Remove data" in command.help_string | ||
| assert command.docs_url is not None | ||
|
|
||
| def test_configure_parser(self): | ||
| command = ForgetCommand() | ||
| parser = argparse.ArgumentParser() | ||
|
|
||
| command.configure_parser(parser) | ||
|
|
||
| actions = {action.dest: action for action in parser._actions} | ||
| assert "dataset" in actions | ||
| assert "dataset_id" in actions | ||
| assert "data_id" in actions | ||
| assert "everything" in actions | ||
| assert "memory_only" in actions | ||
| assert actions["memory_only"].default is False | ||
|
|
||
| @patch("cognee.cli.commands.forget_command.asyncio.run", side_effect=_mock_run) | ||
| def test_execute_threads_memory_only_flag(self, mock_asyncio_run): | ||
| """--memory-only must reach cognee.forget(memory_only=True).""" | ||
| mock_cognee = MagicMock() | ||
| mock_cognee.forget = AsyncMock( | ||
| return_value={"status": "success", "dataset_id": "ds", "data_records_reset": 0} | ||
| ) | ||
|
|
||
| with patch.dict(sys.modules, {"cognee": mock_cognee}): | ||
| command = ForgetCommand() | ||
| args = argparse.Namespace( | ||
| dataset="my_dataset", | ||
| dataset_id=None, | ||
| data_id=None, | ||
| everything=False, | ||
| memory_only=True, | ||
| ) | ||
| command.execute(args) | ||
|
|
||
| mock_cognee.forget.assert_awaited_once_with( | ||
| data_id=None, | ||
| dataset="my_dataset", | ||
| dataset_id=None, | ||
| everything=False, | ||
| memory_only=True, | ||
| ) | ||
|
|
||
| def test_execute_everything_with_memory_only_errors(self): | ||
| """--memory-only has no effect with --everything (which deletes | ||
| outright) -- must error instead of silently doing a full wipe.""" | ||
| mock_cognee = MagicMock() | ||
| mock_cognee.forget = AsyncMock() | ||
|
|
||
| with patch.dict(sys.modules, {"cognee": mock_cognee}): | ||
| command = ForgetCommand() | ||
| args = argparse.Namespace( | ||
| dataset=None, dataset_id=None, data_id=None, everything=True, memory_only=True | ||
| ) | ||
| # Should not raise, just print an error and return without calling forget(). | ||
| command.execute(args) | ||
|
|
||
| mock_cognee.forget.assert_not_awaited() | ||
|
|
||
| def test_execute_no_forget_target(self): | ||
| command = ForgetCommand() | ||
| args = argparse.Namespace( | ||
| dataset=None, dataset_id=None, data_id=None, everything=False, memory_only=False | ||
| ) | ||
|
|
||
| # Should not raise, just print an error and return. | ||
| command.execute(args) | ||
|
|
||
| @patch("cognee.cli.commands.forget_command.asyncio.run") | ||
| def test_execute_with_exception(self, mock_asyncio_run): | ||
| mock_asyncio_run.side_effect = Exception("Forget error") | ||
|
|
||
| command = ForgetCommand() | ||
| args = argparse.Namespace( | ||
| dataset="my_dataset", | ||
| dataset_id=None, | ||
| data_id=None, | ||
| everything=False, | ||
| memory_only=False, | ||
| ) | ||
|
|
||
| with pytest.raises(CliCommandException): | ||
| command.execute(args) | ||
|
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. [blocker] Missing test for the main bug fix — Add a test in this class to match the validation at def test_execute_data_id_without_dataset_errors(self):
"""--data-id alone must error; requires --dataset or --dataset-id."""
mock_cognee = MagicMock()
mock_cognee.forget = AsyncMock()
with patch.dict(sys.modules, {"cognee": mock_cognee}):
command = ForgetCommand()
args = argparse.Namespace(
dataset=None,
dataset_id=None,
data_id="11111111-1111-1111-1111-111111111111",
everything=False,
memory_only=False,
)
# Should not raise, just print an error and return without calling forget().
command.execute(args)
mock_cognee.forget.assert_not_awaited() |
||
|
|
||
|
|
||
| class TestConfigCommand: | ||
| """Test the ConfigCommand class""" | ||
|
|
||
|
|
||
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.
minor — Validation gap:
--data-idwithout a dataset passes through here but fails in the SDK with "data_id requires dataset or dataset_id." For consistency with the other validations added in this PR, checkdata_id and not dataset and not dataset_idand error with "Specify --dataset or --dataset-id when using --data-id."