DM-24515: improvements to deletion (and fixes for bugs found along the way) #261
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
timj
reviewed
Apr 23, 2020
python/lsst/daf/butler/_butler.py
Outdated
| try: | ||
| # Point of no return for removing artifacts | ||
| self.datastore.emptyTrash() | ||
| except BaseException as err: |
There was a problem hiding this comment.
emptyTrash is only going to fail if ctrl-C is hit since by default ignore_errors=True.
python/lsst/daf/butler/_butler.py
Outdated
| # Point of no return for removing artifacts | ||
| self.datastore.emptyTrash() | ||
| except BaseException as err: | ||
| raise IOError( |
There was a problem hiding this comment.
Again this will only happen if something like ctrl-C occurs.
andy-slac
approved these changes
Apr 24, 2020
python/lsst/daf/butler/_butler.py
Outdated
| are fully removed from the data repository. | ||
| purge : `bool`, optional | ||
| If `True`, permit `~CollectionType.RUN` collections to be removed, | ||
| full removing datasets within them. Requires ``unstore=True`` as |
| if unstore: | ||
| for ref in self.registry.queryDatasets(..., collections=name, deduplicate=True): | ||
| if self.datastore.exists(ref): | ||
| self.datastore.trash(ref) |
There was a problem hiding this comment.
Just a general note for the future - for large collections it may be more efficient to implement "bulk trash" operation in data store.
There was a problem hiding this comment.
I completely agree - and unless we change the transaction nesting approach instead, this will be a lot of SAVEPOINTs.
python/lsst/daf/butler/_butler.py
Outdated
| with in `put` and `ingest`, and disassociates from in `prune` (`tuple` of | ||
| `str`). | ||
| with in `put` and `ingest`, and disassociates from in `pruneDatasets` | ||
| (`tuple` of `str`). |
There was a problem hiding this comment.
I think this should be:
`tuple` [`str`]
as described in https://developer.lsst.io/python/numpydoc.html#py-docstring-parameter-types-sequences. Probably not worth changing now if it's consistent within this file.
| ----- | ||
| If this is a `~CollectionType.RUN` collection, all datasets and quanta | ||
| in it are also fully removed. This requires that those datasets be | ||
| removed from any datastores that hold them first. |
There was a problem hiding this comment.
"removed" as in sent to trash? I think datastore has both remove and trash methods, butler does trash and then removeCollection.
There was a problem hiding this comment.
Yes, sent to trash is sufficient. Will clarify.
| nullable=False, | ||
| doc="A link to the associated Dataset.", | ||
| nullable=True, | ||
| doc="A link to the associated dataset; null if the dataset has been completed.", |
There was a problem hiding this comment.
Should be "deleted"; don't know how that happened.
tests/test_butler.py
Outdated
| butler.registry.associate(tag1, [ref3]) | ||
| # Add a CHAINED collection that searches run1 and then run2. It | ||
| # logically contains only ref1, because ref2 is shadowed due to them | ||
| # having the same data ID and dataset tpe. |
tests/test_butler.py
Outdated
| butler.pruneCollection(tag1, purge=True, unstore=True) | ||
| with self.assertRaises(TypeError): | ||
| butler.pruneCollection(chain1, purge=True, unstore=True) | ||
| # Remove the tagged collection with unstore=False. This should should |
A parent dataset can only have one child dataset for a particular component name. It might be a little weird for it to have the same child dataset satisfy multiple components, but that's not actually a problem.
Now that dataset_location_trash can hold dataset IDs that have been deleted from the main table, we need to make sure we don't reuse those deleted ID values. Most database autoincrement behavior never does that, but SQLite's does recycle them unless you tell in not to.
We believe the implementation is now safe, even in the presence of concurrent deletes.
The len(collections) call that was removed here was a bug: it could be testing the length of a string collection name rather than the actual number of collections. The addition of code to standardize the collections expression at the top of queryDatasets doesn't quite fix that, because what we actually care about is the number of recursively-expanded child collections when CHAINED collections are in play, so the safe thing to do is just to remove that check. But I've also kept the code to standardize the collections anyway - it means we only do that once when queryDatasets self-recurses to handle different dataset types.
pruneCollection is coming.
We now just set dataset_consumer column values to NULL instead of attempting to delete full quanta when a dataset they use is deleted. This still clearly marks those quanta as incomplete while allowing us to rely on (safer, more declarative, probably more performance) in-database ON DELETE clauses rather than Python logic. There were no tests for how we handle quanta on deletion before, and I'm not adding any now, because my goal is just to simplify as much as possible ahead of the dataset-table refactor of DM-21764. We can improve our test coverage (and functionality) later when we have real code that exercises the database quantum tables.
This should save us a lot of SAVEPOINT calls, though it will still leave us with at least one per deleted dataset because the datastore deletion interfaces are not yet vectorized.
TallJimbo
force-pushed
the
tickets/DM-24515
branch
from
2160596
to
8ba5bd1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.