ENG-2213: Remove deprecated DSR 2.0 code#7438
Conversation
DSR 3.0 is now the only execution path. This removes the deprecated DSR 2.0 sequential/Dask-based execution model including: - deprecated_graph_task.py and scheduler_utils.py modules - Redis caching methods from TaskResources - DSR 2.0 fallback paths in privacy_request model methods - to_mock_execution_node() from traversal.py - use_dsr_2_0 test fixture and all DSR version parameterization - Deleted test files: test_deprecated_graph_task, test_task_resources, test_dsr_3_0_default Moved format_data_use_map_for_caching() to create_request_tasks.py as it is still used by DSR 3.0. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Co-authored-by: Cursor <cursoragent@cursor.com>
- Restore to_mock_execution_node() on TraversalNode (used by test infrastructure, not DSR 2.0 execution) - Remove test_dsr_2_0 methods from test_request_task.py (tested deleted Redis cache fallback) - Remove test_scylladb_access_request_task_no_keyspace_dsr2 (the _dsr3 variant already covers this scenario) Co-authored-by: Cursor <cursoragent@cursor.com>
Restore the request_task.id check in GraphTask.update_status() to skip db.merge for in-memory mock RequestTask objects used by tests. Co-authored-by: Cursor <cursoragent@cursor.com>
The retry decorator no longer re-raises exceptions for non-persistent RequestTasks. Update test to verify the error is caught and logged rather than propagated. Co-authored-by: Cursor <cursoragent@cursor.com>
These tests called access_runner/erasure_runner directly which now always raise PrivacyRequestExit. Their docstrings noted the DSR 3.0 equivalents exist in test_create_request_tasks.py. Co-authored-by: Cursor <cursoragent@cursor.com>
DSR 3.0 is the only execution path. Remove all dsr_version/use_dsr_3_0 test parameterization, conditional DSR version logic, and the no-op use_dsr_3_0 fixture and config setting across 44 files. Co-authored-by: Cursor <cursoragent@cursor.com>
Greptile SummaryThis PR successfully removes all deprecated DSR 2.0 code, making DSR 3.0 the sole execution path for data subject requests. The changes are purely subtractive (3,737 deletions vs 537 additions), with the additions being primarily relocated code and test cleanup. Key changes:
PR description minor inaccuracy: All imports have been verified clean - no references to deleted modules remain. The changes are well-structured and maintain backward compatibility where needed (DSR 3.0 execution path unchanged). Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 38ffb72 |
f782d37 to
38ffb72
Compare
JadeCara
left a comment
There was a problem hiding this comment.
A few comments etc
- This one is from claude - I think its reasobable though:
The || has lower precedence than &&, so if isVerificationRequired is false and data.succeeded is empty, data.succeeded[0].status === DUPLICATE will throw a TypeError. Fix: wrap the second branch in a
length guard.
(Pre-existing bug from PR #7238, but the surrounding code changes make this path more reachable.)
-
There are still a few docstrings where 3.0 is referenced. Do we want to keep that since it is TheWay ™️ now?
-
Codegraph found some dead code left over - please verify since I am still finding edges.
Codegraph Dead Code Report
Confirmed Dead Code (safe to delete)
Function: PrivacyRequest.get_results
Location: privacy_request.py:825
Callers: 0 callers
Impact (dependents): 0 dependents
Neighbourhood: Outbound only: calls get_cache() → FidesopsRedis. No inbound edges from any fides or fidesplus code.
Verdict: Dead code. Docstring even says "Just used in testing" but no tests call it anymore.
────────────────────────────────────────
Function: extract_key_for_address
Location: collection_util.py:107
Callers: 0 callers
Impact (dependents): 0 dependents
Neighbourhood: No neighbours at all (completely isolated node).
Verdict: Dead code. Total orphan - zero inbound, zero outbound connections.
────────────────────────────────────────
Function: start_function
Location: graph_task.py:936
Callers: 0 callers
Impact (dependents): 0 dependents
Neighbourhood: No neighbours at all (completely isolated node).
Verdict: Dead code. Was used for Dask graph seed nodes. Total orphan.
src/fides/api/task/graph_runners.py
Outdated
| raise PrivacyRequestExit() | ||
|
|
||
| return run_access_request_deprecated( | ||
| """Run an access request using DSR 3.0 task-based execution.""" |
There was a problem hiding this comment.
Can we drop the 3.0 from the docstring here as well?
There was a problem hiding this comment.
Done — removed "DSR 3.0" from all three runner docstrings in b5212b5.
| self, | ||
| db, | ||
| dsr_version, | ||
| request, |
There was a problem hiding this comment.
I think its safe to remove this here now
| request, |
There was a problem hiding this comment.
Investigated — these two tests cover live behavior (enabled_actions filtering on manual webhooks within run_privacy_request). No other integration test covers this path; the model-level unit test in test_manual_webhook.py only tests AccessManualWebhook.get_enabled() directly, not the full pipeline pause/skip. Kept the tests but removed the unused request fixture param left over from DSR version parameterization.
| self, | ||
| db, | ||
| dsr_version, | ||
| request, |
There was a problem hiding this comment.
I think its safe to remove this here now as well.
| request, |
There was a problem hiding this comment.
Same as above — kept the test since it covers unique behavior, but cleaned up the unused request fixture param.
There was a problem hiding this comment.
Removed in b5212b5 — simplified to just "Restart from the checkpoint".
src/fides/api/task/graph_task.py
Outdated
There was a problem hiding this comment.
looks like this comment got cut off - we can probably remove the reference to 3.0 since that is TheWay ™️ now
There was a problem hiding this comment.
Fixed — replaced the cut-off comment with "Saved as part of the request task" in b5212b5.
There was a problem hiding this comment.
I havent fully verified this admittedly - but claude and the graph called this out
Finding #7: Unused parameters — Confirmed
erasure_runner — 5 unused params confirmed. graph_file for create_request_tasks.py shows run_erasure_request (line 646) has edges only to:
- PrivacyRequest (REFERENCES)
- RequestTask (REFERENCES)
- queue_request_task (CALLS)
- log_task_queued (CALLS)
No edges to Policy, DatasetGraph, ConnectionConfig, or any identity-related node. The runner passes only privacy_request, session, and privacy_request_proceed — the other 5 params (policy, graph,
connection_configs, identity, access_request_data) are accepted but never forwarded.
consent_runner — 2 unused params confirmed. run_consent_request (line 669) has edges to:
- DatasetGraph (REFERENCES) — uses graph param
- PrivacyRequest (REFERENCES)
- RequestTask (REFERENCES)
- TraversalNode (CALLS)
- get_connection_configs_with_manual_tasks (CALLS) — fetches its own configs
- queue_request_task (CALLS)
- log_task_queued (CALLS)
No edges to Policy or ConnectionConfig. The runner accepts policy and connection_configs but never passes them through.
access_runner — clean. run_access_request (line 539) references Policy, DatasetGraph, ConnectionConfig, PrivacyRequest, RequestTask, and Traversal — all params are used.
Cross-repo impact: graph_context confirmed callers in fidesplus — erasure_runner_tester and consent_runner_tester in test helpers, plus ConnectorRunner methods. Any signature cleanup would need coordinated
changes.
---
Finding #8: Misleading return types — Confirmed
All three runners unconditionally raise PrivacyRequestExit() after delegating to run_*_request. The declared return types are unreachable:
- access_runner → Dict[str, List[Row]] (never returned)
- erasure_runner → Dict[str, int] (never returned)
- consent_runner → Dict[str, bool] (never returned)
The test wrappers in tests/conftest.py (lines 930, 960, 989) catch PrivacyRequestExit and return actual results from the database, which is why tests still work. The return types on the runners themselves
are vestigial from the DSR 2.0 era when these functions returned results directly.
A more accurate signature would be -> NoReturn from typing, since the functions always raise.
---
Both findings are solid and codegraph-verified. Neither is a bug per se — they're cleanup opportunities left behind by the DSR 2.0 removal. The unused params are kept for call-site compatibility
(especially cross-repo), and the return types are harmless but misleading.
There was a problem hiding this comment.
Addressed in two parts:
Dead code — deleted in b5212b5:
PrivacyRequest.get_results— confirmed zero callersextract_key_for_address— confirmed zero callersstart_function— confirmed zero callers
Unused params / misleading return types — deferring to follow-up PR:
The erasure_runner (5 unused params) and consent_runner (2 unused params) cleanup requires coordinated changes across fides (request_runner_service.py, tests/conftest.py) and fidesplus (saas_test_utils.py + ~20 SaaS test files). Same for updating return types to NoReturn. Will track as a separate cleanup ticket.
- Remove "DSR 3.0" / "DSR 2.0" from docstrings and comments in graph_runners, graph_task, test_enabled_actions, and test_execution - Remove unused `request` fixture param from manual webhook tests - Delete dead code: PrivacyRequest.get_results, extract_key_for_address, start_function (all confirmed zero callers) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
All inline comments addressed in b5212b5. Summary:
|
Co-authored-by: Adrian Galvan <galvana@uci.edu> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Ticket ENG-2213
Description Of Changes
Remove all deprecated DSR 2.0 code, making DSR 3.0 the sole execution path for data subject requests. DSR 2.0 was a sequential, in-memory execution model using Dask with Redis caching. It has been deprecated since DSR 3.0 (task-based async execution via RequestTasks) became the default.
This is a purely subtractive change — no new functionality is introduced. The
use_dsr_3_0fixture is kept as a no-op for backward compatibility with fidesplus tests.Code Changes
Deleted modules:
src/fides/api/task/deprecated_graph_task.py- Core DSR 2.0 sequential execution logicsrc/fides/api/task/scheduler_utils.py- DSR 2.0/3.0 routing logic (use_dsr_3_0_scheduler())Deleted test files:
tests/ops/task/test_deprecated_graph_task.pytests/ops/task/test_task_resources.pytests/test_dsr_3_0_default.pySimplified source files:
src/fides/api/task/graph_runners.py- Unconditionally delegates to DSR 3.0 and raisesPrivacyRequestExitsrc/fides/api/task/task_resources.py- Removed DSR 2.0 Redis caching methods (cache_results_with_placeholders,cache_object,get_all_cached_objects,cache_erasure,get_all_cached_erasures)src/fides/api/task/graph_task.py- Removed DSR 2.0 caching calls and conditional blockssrc/fides/api/task/create_request_tasks.py- Receivedformat_data_use_map_for_caching()relocated from deprecated_graph_task (still used by DSR 3.0)src/fides/api/models/privacy_request/privacy_request.py- Removed DSR 2.0 Redis cache fallback pathssrc/fides/api/graph/traversal.py- Removedto_mock_execution_node()(DSR 2.0 only).fides/redis_dataset.yml- Removed DSR 2.0 cache key documentationTest updates (~40 files):
use_dsr_2_0fixture and all"use_dsr_2_0"parametrizationif dsr_version == "use_dsr_2_0"conditional branchesget_cached_data_for_erasures(...)calls with{}use_dsr_3_0to a no-op fixture for backward compatibilitySteps to Confirm
nox -s static_checkspasses (ruff check, ruff format, mypy)python -c "from fides.api.task.graph_runners import access_runner; from fides.api.task.create_request_tasks import format_data_use_map_for_caching"python -c "import fides.api.task.deprecated_graph_task"should raiseImportErrorPre-Merge Checklist
CHANGELOG.mdupdatedMade with Cursor