From 7d48c6227f7230493261cdfcc52d5667ab159c30 Mon Sep 17 00:00:00 2001
From: Chris Brozdowski <Chris.Broz@ucsf.edu>
Date: Fri, 4 Aug 2023 14:57:59 -0700
Subject: [PATCH] Merge delete parent fix (#617)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* Restructure for mkdocs

* Minor docstring edits for mkdocs

* Adjust installer docs

* Permit json config

* Update docstrings for mkdocs. Add images

* Minor fixes. Rename publish action. Add changelog notes

* hard wrap changes, markdownlint

* blackify

* Fix version cmd. Minor doc wording

* mkdocstrings require empty inits to identify submodules

* 🤦‍♂️ Publish docs CI/CD `main`->`master`

* Edit `get_part`; Add `merge_fetch`

* Spelling fixes

* Refactor restrict_parts. Adjust mkdocs nav.

* Docs adjust for Merge tables

* Fix `merge_get_part`

* Use hatch for docs version

* Update changelog

* Typo

* Spellcheck config

* WIP: dict/str ristrict consistency

* Normalize restriction/classmeth. Add notes on why

* Typos

* Docs publish on tag

* Edit changelog: Add links, patch version bump

* 🧪 Test gh-actions debug

* get-part multi-source flag

* Add mutual exclusivity flag from pos branch

* See details. Notebook work, config overhaul

gitignore: add exclude example config
mkdocs: new notebook names
notebooks: complete revamp for minirec data and more links to docs
init: add new load_config, isort imports
common_lab:
	- adjust to accept names in Last, First format (nwb-compliant)
	- continue to use First Last name structure in database - yes?
common_nwb: use new load_config, change `assert` to `raise`
insert_sessions: permit paths, use file name, use raw dir
storage_dirs: remove redundant funcs for base_dir
settings: implement new base_dir system
	- allows base/raw/etc to be independent
	- defaults to dj.config, then env vars, then sets default rel paths

* WIP: fix failing tests related to base_dir edits

* underscore-prefix Merge. Linter fixes

* See Details. Notebook overhaul

- dj_config: accept base dir as arg, refactor for single responsibility
- mkdocs, installation.md
  - condense installation information to single page
  - reference new notebook
  - remove local and production subpages due to redundancy
- environment and env_position.yml: add install current dir to avoid
  additional step in installation process
- notebooks: rewrite with Docker optional and minirec as demo data
- common_lab: raise error for invalid name
- common_position: get raw dir from settings, not hardcode
- settings.py: Should this be a class with properties?
  - add options for kachery dirs set via same dj_config mechanism
  - add raw_dir helper function

* remove note to self

* WIP notebook edits

* Revise 04_LFP nb

* Fix #614

* Fix #609
---
 src/spyglass/utils/dj_merge_tables.py | 37 ++++++++++++++++-----------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/src/spyglass/utils/dj_merge_tables.py b/src/spyglass/utils/dj_merge_tables.py
index 159ec46ad..cc7dccd89 100644
--- a/src/spyglass/utils/dj_merge_tables.py
+++ b/src/spyglass/utils/dj_merge_tables.py
@@ -208,12 +208,16 @@ def _merge_repr(cls, restriction: str = True) -> dj.expression.Union:
             )
         ]
 
+        if not parts:
+            print(f"No entries found for restriction: {restriction}")
+            return
+
         primary_attrs = list(
             dict.fromkeys(  # get all columns from parts
                 iter_chain.from_iterable([p.heading.names for p in parts])
             )
         )
-        # primary_attrs.append(cls()._reserved_sk)
+
         query = dj.U(*primary_attrs) * parts[0].proj(  # declare query
             ...,  # include all attributes from part 0
             **{
@@ -445,10 +449,19 @@ def merge_delete_parent(
         if dry_run:
             return part_parents
 
-        with cls._safe_context():
-            super().delete(cls(), **kwargs)
-            for part_parent in part_parents:
-                super().delete(part_parent, **kwargs)
+        merge_ids = cls.merge_restrict(restriction).fetch(
+            RESERVED_PRIMARY_KEY, as_dict=True
+        )
+
+        # CB: Removed transaction protection here bc 'no' confirmation resp
+        # still resulted in deletes. If re-add, consider transaction=False
+        super().delete((cls & merge_ids), **kwargs)
+
+        if cls & merge_ids:  # If 'no' on del prompt from above, skip below
+            return  # User can still abort del below, but yes/no is unlikly
+
+        for part_parent in part_parents:
+            super().delete(part_parent, **kwargs)  # add safemode=False?
 
     @classmethod
     def fetch_nwb(
@@ -541,8 +554,8 @@ def merge_get_part(
 
         if not multi_source and len(sources) != 1:
             raise ValueError(
-                f"Found multiple potential parts: {sources}\n\t"
-                + "Try adding a restriction before invoking `get_part`.\n\t"
+                f"Found {len(sources)} potential parts: {sources}\n\t"
+                + "\n\tTry adjusting the restriction. "
                 + "Or permitting multiple sources with `multi_source=True`."
             )
 
@@ -593,8 +606,8 @@ def merge_get_parent(
 
         if not multi_source and len(part_parents) != 1:
             raise ValueError(
-                f"Found multiple potential parents: {part_parents}\n\t"
-                + "Try adding a string restriction when invoking `get_parent`."
+                f"Found {len(part_parents)} possible parents: {part_parents}"
+                + "\n\tTry adjusting the restriction. "
                 + "Or permitting multiple sources with `multi_source=True`."
             )
 
@@ -664,12 +677,6 @@ def merge_populate(source: str, key=None):
 # Aliased because underscore otherwise excludes from API docs.
 
 
-_Merge = Merge
-
-# Underscore as class name avoids errors when this included in a Diagram
-# Aliased because underscore otherwise excludes from API docs.
-
-
 def delete_downstream_merge(
     table: dj.Table, restriction: str = True, dry_run=True, **kwargs
 ) -> list: