diff --git a/README.md b/README.md index 27d693b..46f78d1 100644 --- a/README.md +++ b/README.md @@ -16,6 +16,27 @@ n.b. View individual files for informative docstrings and other usage comments. ### Dataservice +#### [dataservice/delete.py](kf_utils/dataservice/delete.py) - Study deletion + +```Python +from kf_utils.dataservice.delete import delete_entities +``` + +```Python +# Delete all studies in Dataservice (raises Exception on deletion of non-local resources) +delete_entities(kf_api_url, study_ids=None) +``` + +```Python +# Delete a study in Dataservice (raises Exception on deletion of non-local resources) +delete_entities(kf_api_url, study_ids=["SD_12345678"]) +``` + +```Python +# Delete a study in Dataservice (allows deletion of non-local resources) +delete_entities(kf_api_url, study_ids=["SD_12345678"], safety_check=False) +``` + #### [dataservice/scrape.py](kf_utils/dataservice/scrape.py) - Query scraping The Kids First dataservice paginates its responses and can only return up @@ -24,7 +45,6 @@ of the entities from all of the pages for a given query. ```Python from kf_utils.dataservice.scrape import * -from kf_utils.dataservice.delete import delete_entities ``` ```Python @@ -64,17 +84,6 @@ for kfid in yield_kfids( ... ``` -```Python -# Delete all studies in Dataservice (raises Exception on deletion of non-local resources) -delete_entities(kf_api_url, study_ids=None) - -# Delete a study in Dataservice (raises Exception on deletion of non-local resources) -delete_entities(kf_api_url, study_ids=["SD_12345678"]) - -# Delete a study in Dataservice (allows deletion of non-local resources) -delete_entities(kf_api_url, study_ids=["SD_12345678"], safety_check=False) -``` - #### [dataservice/descendants.py](kf_utils/dataservice/descendants.py) - Descendant entity discovery Entities in the dataservice are linked to each other by descendancy diff --git a/kf_utils/dataservice/delete.py b/kf_utils/dataservice/delete.py index 9098914..4a8ec30 100644 --- a/kf_utils/dataservice/delete.py +++ b/kf_utils/dataservice/delete.py @@ -1,10 +1,9 @@ -import logging +from concurrent.futures import ThreadPoolExecutor from pprint import pformat from urllib.parse import urlparse -import requests from d3b_utils.requests_retry import Session - +from kf_utils.dataservice.meta import get_endpoint from kf_utils.dataservice.scrape import yield_kfids # DO NOT RE-ORDER - deletion requires this order @@ -29,72 +28,36 @@ "127.0.0.1", } -logger = logging.getLogger(__name__) - -def safe_delete(url, safety_check=True, session=None, **kwargs): +def delete_kfids(host, kfids, safety_check=True): """ - Only delete a non-local resource if safety_check is False. - - :param url: url of resource that will be deleted - :type url: url - :param saftey_check: Whether to delete if resource is not at localhost - :type safety_check: bool - :param session: requests session object to use when deleting - :type session: requests.Session - :param kwargs: Keyword args passed to requests.Session.delete - :type kwarrgs: dict + Rapidly delete entities by KF ID. Default behavior only deletes resources + at localhost unless safety_check=False - :returns: requests.Response from the delete - :raises: Exception if safety_check=True and url is non-local + :param host: dataservice base url string (e.g. "http://localhost:5000") + :type host: str + :param kfids: Data Service Kids First IDs + :type kfids: iterable of strs """ - host = urlparse(url).netloc.split(":")[0] - if safety_check and (host not in LOCAL_HOSTS): + kfids = list(kfids) + host = host.strip("/") + base = urlparse(host).netloc.split(":")[0] + if safety_check and (base not in LOCAL_HOSTS): raise Exception( - f"Cannot delete {url}. Safe delete is ENABLED. Resources that are " - "not on localhost will not be deleted. You can try again with " - "safe delete disabled." + f"Cannot delete from {host} because safety_check is ENABLED. " + f"Resources that are not in {LOCAL_HOSTS} will not be deleted " + "unless you set safety_check=False." ) - session = session or Session() - resp = session.delete(url, **kwargs) - return resp - - -def delete_kfids(host, endpoint, kfids, safety_check=True): - """ - Delete entities by KF ID. Default behavior only deletes resources at - localhost unless safety_check=False - - :param host: URL of the Data Service - :type host: str - :param endpoint: Data Service endpoint - :type endpoint: str - :param kfids: Data Service Kids First IDs - :type kfids: iterable of strs - :param saftey_check: Whether to delete if resource is not at localhost - :type safety_check: bool + def delete(u): + return Session().delete(u) - :returns: Dict of errors where Keys are urls that failed delete (non 200 - status code) and values are requests.Response objects - """ - session = Session() - errors = {} - kf_ids = [kfid for kfid in kfids] - total = len(kf_ids) - for i, kf_id in enumerate(kf_ids): - url = f"{host}/{endpoint.strip('/')}/{kf_id}" - logger.info(f"Deleting {i+1} of {total}: {url}") - resp = safe_delete(url, session=session, safety_check=safety_check) - try: - resp.raise_for_status() - except requests.exceptions.HTTPError: - logger.error( - f"Failed to delete {url}, status code {resp.status_code}. " - f"Response:\n{resp.text}" - ) - errors[url] = resp - return errors + total = len(kfids) + with ThreadPoolExecutor(max_workers=5) as tpex: + for i, f in enumerate( + tpex.map(delete, [f"{host}/{get_endpoint(k)}/{k}" for k in kfids]) + ): + print(f"Deleted {i+1} of {total}: {f.url}") def delete_entities(host, study_ids=None, safety_check=True): @@ -107,7 +70,7 @@ def delete_entities(host, study_ids=None, safety_check=True): Deletion is implemented in a way that avoids large cascading deletions in the Data Service database. Large cascading deletes are known to crash the Data Service. For example, first we delete genomic files, then - biospecimens, and then participants rather than deleting participants + biospecimens, and then participants rather than deleting participants first since that would cause a cascading delete of the specimens and their genomic files. The order in which entities are deleted is defined in ENDPOINTS. @@ -119,30 +82,42 @@ def delete_entities(host, study_ids=None, safety_check=True): :type study_ids: list of str :param saftey_check: Whether to delete if resource is not at localhost :type safety_check: bool - - :returns: Dict of errors where Keys are urls that failed delete (non 200 - status code) and values are requests.Response objects """ phrase = f"studies {pformat(study_ids)}" if study_ids else "all studies" - logger.info(f"Deleting {phrase} from {host}") - - # Get all study ids - if not study_ids: - study_ids = yield_kfids(host, "studies", {}) - - # Delete entities by study id - errors = {} - for study_id in study_ids: - # Delete entities except "study" (it has to be handled differently) + print(f"Deleting {phrase} from {host}") + + if study_ids: + # Delete entities by study id + for study_id in study_ids: + # Delete entities except "study" (it has to be handled differently) + for endpoint in ENDPOINTS: + print(f"Finding all {endpoint} from study {study_id}.") + kfids = yield_kfids( + host, endpoint, {"study_id": study_id}, show_progress=True + ) + if kfids: + print(f"Deleting all {endpoint} from study {study_id}.") + delete_kfids(host, kfids, safety_check=safety_check) + else: + print(f"No {endpoint} found.") + + # Delete study by its kfid + delete_kfids(host, [study_id], safety_check=safety_check) + else: + # Delete everything for endpoint in ENDPOINTS: - params = {"study_id": study_id} - kfids = yield_kfids(host, endpoint, params) - errors.update( - delete_kfids(host, endpoint, kfids, safety_check=safety_check) - ) - # Delete study by its kfid - errors.update( - delete_kfids(host, STUDIES, [study_id], safety_check=safety_check) - ) - - return errors + print(f"Finding all {endpoint}.") + kfids = yield_kfids(host, endpoint, {}, show_progress=True) + if kfids: + print(f"Deleting all {endpoint}.") + delete_kfids(host, kfids, safety_check=safety_check) + else: + print(f"No {endpoint} found.") + + print(f"Finding all {STUDIES}.") + kfids = yield_kfids(host, STUDIES, {}, show_progress=True) + if kfids: + print(f"Deleting all {STUDIES}.") + delete_kfids(host, kfids, safety_check=safety_check) + else: + print(f"No {STUDIES} found.") diff --git a/kf_utils/dataservice/patch.py b/kf_utils/dataservice/patch.py index 6704059..5c727c8 100644 --- a/kf_utils/dataservice/patch.py +++ b/kf_utils/dataservice/patch.py @@ -12,6 +12,7 @@ def send_patches(host, patches): :param patches: dict mapping KFIDs to patch dicts :raises Exception: if server doesn't respond OK """ + host = host.strip("/") def do_patch(url, patch): msg = f"Patched {url} with {patch}" diff --git a/kf_utils/dataservice/scrape.py b/kf_utils/dataservice/scrape.py index 04faebd..a39bad9 100755 --- a/kf_utils/dataservice/scrape.py +++ b/kf_utils/dataservice/scrape.py @@ -2,6 +2,7 @@ from d3b_utils.requests_retry import Session from kf_utils.dataservice.meta import get_endpoint +from tqdm import tqdm def yield_entities_from_filter(host, endpoint, filters, show_progress=False): @@ -26,30 +27,35 @@ def yield_entities_from_filter(host, endpoint, filters, show_progress=False): found_kfids = set() which = {"limit": 100} expected = 0 - while True: - resp = Session().get(url, params={**which, **filters}) - - if resp.status_code != 200: - raise Exception(resp.text) - - j = resp.json() - res = j["results"] - expected = j["total"] - - if show_progress and not res: - print("o", end="", flush=True) - for entity in res: - kfid = entity["kf_id"] - if kfid not in found_kfids: - found_kfids.add(kfid) - if show_progress: - print(".", end="", flush=True) - yield entity - try: - for (key, i) in [("after", 1), ("after_uuid", 2)]: - which[key] = j["_links"]["next"].split("=")[i].split("&")[0] - except KeyError: - break + with tqdm(total=1, disable=not show_progress, leave=False) as pbar: + while True: + resp = Session().get(url, params={**which, **filters}) + + if resp.status_code != 200: + raise Exception(resp.text) + + j = resp.json() + if j["total"] != expected: + n = pbar.n + pbar.reset(j["total"]) + pbar.update(n) + + expected = j["total"] + res = j["results"] + + if not res: + pbar.close() + for entity in res: + kfid = entity["kf_id"] + if kfid not in found_kfids: + found_kfids.add(kfid) + pbar.update() + yield entity + try: + for (key, i) in [("after", 1), ("after_uuid", 2)]: + which[key] = j["_links"]["next"].split("=")[i].split("&")[0] + except KeyError: + break num = len(found_kfids) assert expected == num, f"FOUND {num} ENTITIES BUT EXPECTED {expected}" @@ -65,7 +71,11 @@ def yield_entities_from_kfids(host, kfids, show_progress=False): """ host = host.strip("/") + quit = False + def do_get(url): + if quit: + return response = Session().get(url) if response.status_code != 200: raise Exception(response.text) @@ -78,10 +88,16 @@ def do_get(url): futures = [ tpex.submit(do_get, f"{host}/{get_endpoint(k)}/{k}") for k in kfids ] - for f in as_completed(futures): - if show_progress: - print(".", end="", flush=True) - yield f.result() + with tqdm(total=len(kfids), disable=not show_progress) as pbar: + try: + for f in as_completed(futures): + pbar.update() + yield f.result() + except KeyboardInterrupt: + quit = True + tpex.shutdown(wait=False) + tpex._threads.clear() + raise def yield_entities( diff --git a/requirements.txt b/requirements.txt index 78971e4..730eb40 100644 --- a/requirements.txt +++ b/requirements.txt @@ -2,3 +2,4 @@ d3b_utils @ git+https://github.com/d3b-center/d3b-utils-python.git # defusedxml xmltodict psycopg2-binary +tqdm diff --git a/tests/conftest.py b/tests/conftest.py index bcdd24d..8cda169 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -25,7 +25,7 @@ def create_sequencing_center(): def create_study(si): - kfid = f"SD_{si}1111111" + kfid = f"SD_{si}".ljust(11, "1") requests.post( f"{DATASERVICE_URL}/studies", json={"kf_id": kfid, "external_id": f"{si}"}, @@ -34,12 +34,12 @@ def create_study(si): def create_participant(si, pi): - kfid = f"PT_{si}{pi}111111" + kfid = f"PT_{si}{pi}".ljust(11, "1") requests.post( f"{DATASERVICE_URL}/participants", json={ "kf_id": kfid, - "study_id": f"SD_{si}1111111", + "study_id": f"SD_{si}".ljust(11, "1"), "external_id": f"{pi}", }, ) @@ -47,12 +47,12 @@ def create_participant(si, pi): def create_biospecimen(si, pi, bi): - kfid = f"BS_{si}{pi}{bi}11111" + kfid = f"BS_{si}{pi}{bi}".ljust(11, "1") requests.post( f"{DATASERVICE_URL}/biospecimens", json={ "kf_id": kfid, - "participant_id": f"PT_{si}{pi}111111", + "participant_id": f"PT_{si}{pi}".ljust(11, "1"), "external_sample_id": f"{pi}{bi}", "external_aliquot_id": f"{pi}{bi}", "sequencing_center_id": "SC_11111111", @@ -62,14 +62,18 @@ def create_biospecimen(si, pi, bi): return kfid -def populate_data(n): +def populate_data(ns, np=None, nb=None): + if np is None: + np = ns + if nb is None: + nb = np # Create some data in dataservice kfids = [] kfids.append(create_sequencing_center()) - for si in range(n): + for si in range(ns): kfids.append(create_study(si)) - for pi in range(n): + for pi in range(np): kfids.append(create_participant(si, pi)) - for bi in range(n): + for bi in range(nb): kfids.append(create_biospecimen(si, pi, bi)) return kfids diff --git a/tests/integration/test_dataservice_delete.py b/tests/integration/test_dataservice_delete.py index 975a294..ef9f706 100644 --- a/tests/integration/test_dataservice_delete.py +++ b/tests/integration/test_dataservice_delete.py @@ -1,4 +1,4 @@ -from tests.conftest import DATASERVICE_URL +from tests.conftest import DATASERVICE_URL, populate_data import requests from kf_utils.dataservice.delete import delete_entities, ENDPOINTS, STUDIES @@ -56,3 +56,10 @@ def test_delete_entities(dataservice_setup): for endpoint in ENDPOINTS + [STUDIES]: resp = requests.get(f"{DATASERVICE_URL}/{endpoint}") assert resp.json()["total"] == 0 + + +def test_delete_many(dataservice_setup): + # iterating the wrong way clogs an executor + # see https://github.com/kids-first/kf-utils-python/pull/36#pullrequestreview-685334120 + populate_data(1, 500, 0) + delete_entities(DATASERVICE_URL) diff --git a/tests/test_dataservice_delete.py b/tests/test_dataservice_delete.py index 56c41b8..f082888 100644 --- a/tests/test_dataservice_delete.py +++ b/tests/test_dataservice_delete.py @@ -6,7 +6,6 @@ from kf_utils.dataservice.delete import ( delete_kfids, delete_entities, - safe_delete, ENDPOINTS, STUDIES, ) @@ -15,34 +14,42 @@ @pytest.mark.parametrize( - "url,safety_check,mysession,kwargs", + "url,safety_check,should_error", [ - ("http://localhost", True, MagicMock(), {"foo": "bar"}), - ("http://localhost", False, None, {"foo": "bar"}), - ("http://127.0.0.1", True, MagicMock(), {"foo": "bar"}), - ("http://127.0.0.1", False, None, {"foo": "bar"}), - ("http://prd.dataservice.org", True, MagicMock(), {"foo": "bar"}), - ("http://prd.dataservice.org", False, None, {"foo": "bar"}), - ("http://10.10.1.191", True, MagicMock(), {"foo": "bar"}), - ("http://10.10.1.191", False, None, {"foo": "bar"}), + ("http://localhost", True, False), + ("http://localhost", False, False), + ("http://localhost:5555", True, False), + ("http://localhost:5555", False, False), + ("http://127.0.0.1", True, False), + ("http://127.0.0.1", False, False), + ("http://127.0.0.1:5555", True, False), + ("http://127.0.0.1:5555", False, False), + ("http://prd.dataservice.org", True, True), + ("http://prd.dataservice.org", False, False), + ("http://prd.dataservice.org:5555", True, True), + ("http://prd.dataservice.org:5555", False, False), + ("http://10.10.1.191", True, True), + ("http://10.10.1.191", False, False), + ("http://10.10.1.191:5555", True, True), + ("http://10.10.1.191:5555", False, False), ], ) -def test_safe_delete(mocker, url, safety_check, mysession, kwargs): +def test_safety_check(mocker, url, safety_check, should_error): """ Test kf_utils.dataservice.delete.safe_delete """ # Setup mocks mock_session = mocker.patch("kf_utils.dataservice.delete.Session")() + mock_resp = MagicMock() + mock_session.delete.return_value = mock_resp + kfids = [f"PT_{i}" for i in range(2)] - if safety_check and ("localhost" not in url): + if should_error: with pytest.raises(Exception) as e: - safe_delete(url, safety_check=safety_check, session=session) - assert "Safe delete is ENABLED" in str(e) + delete_kfids(url, kfids, safety_check=safety_check) + assert "safety_check is ENABLED" in str(e) else: - safe_delete(url, safety_check=safety_check, session=mysession, **kwargs) - if mysession: - mock_session = mysession - mock_session.delete.assert_called_with(url, **kwargs) + delete_kfids(url, kfids, safety_check=safety_check) def test_delete_kfids(mocker): @@ -53,24 +60,14 @@ def test_delete_kfids(mocker): mock_session = mocker.patch("kf_utils.dataservice.delete.Session")() mock_resp = MagicMock() mock_session.delete.return_value = mock_resp - kfids = [f"kfid{i}" for i in range(2)] + kfids = [f"PT_{i}" for i in range(2)] # Successful delete - errors = delete_kfids(HOST, "endpoint", kfids) - assert not errors + delete_kfids(HOST, kfids) assert mock_session.delete.call_count == len(kfids) mock_session.reset_mock() mock_resp.reset_mock() - # Delete with errors - mock_resp.raise_for_status.side_effect = HTTPError - mock_session.delete.return_value = mock_resp - errors = delete_kfids(HOST, "endpoint", kfids) - assert errors - for kfid in kfids: - assert f"{HOST}/endpoint/{kfid}" in errors - assert mock_session.delete.call_count == len(kfids) - def test_delete_entities(mocker): """ @@ -78,21 +75,20 @@ def test_delete_entities(mocker): """ mock_yield_kfids = mocker.patch("kf_utils.dataservice.delete.yield_kfids") mock_delete_kfids = mocker.patch("kf_utils.dataservice.delete.delete_kfids") - kfids = [f"kfid{i}" for i in range(2)] - study_ids = [f"study{i}" for i in range(2)] + kfids = [f"PT_{i}" for i in range(2)] + study_ids = [f"SD_{i}" for i in range(2)] mock_yield_kfids.return_value = kfids mock_delete_kfids.return_value = {} # Delete entities in multiple studies - errors = delete_entities(HOST, study_ids=study_ids) - assert not errors + delete_entities(HOST, study_ids=study_ids) mock_delete_kfids.assert_has_calls( - [call(HOST, STUDIES, [sid], safety_check=True) for sid in study_ids], + [call(HOST, [sid], safety_check=True) for sid in study_ids], any_order=True, ) mock_yield_kfids.assert_has_calls( [ - call(HOST, e, {"study_id": sid}) + call(HOST, e, {"study_id": sid}, show_progress=True) for e in ENDPOINTS for sid in study_ids ], @@ -103,22 +99,14 @@ def test_delete_entities(mocker): # Delete all entities mock_yield_kfids.return_value = study_ids - errors = delete_entities(HOST, study_ids=None) - assert not errors + delete_entities(HOST, study_ids=None) mock_yield_kfids.assert_has_calls( - [call(HOST, STUDIES, {})] + [call(HOST, STUDIES, {}, show_progress=True)] + [ - call(HOST, e, {"study_id": sid}) + call(HOST, e, {}, show_progress=True) for e in ENDPOINTS - for sid in study_ids ], any_order=True, ) mock_yield_kfids.reset_mock() mock_delete_kfids.reset_mock() - - # Delete all entities with error - mock_errors = {"{HOST}/endpoint/{kfid}": "fake resp" for kfid in kfids} - mock_delete_kfids.return_value = mock_errors - errors = delete_entities(HOST, study_ids=study_ids) - assert errors == mock_errors