DM-46799: refactor DatasetRecordStorageManager #1095
Merged
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
TallJimbo
force-pushed
the
tickets/DM-46799
branch
from
e49b8c9
to
a85d17f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1095 +/- ##
==========================================
- Coverage 89.74% 89.69% -0.05%
==========================================
Files 361 360 -1
Lines 47393 47394 +1
Branches 5723 5730 +7
==========================================
- Hits 42532 42512 -20
- Misses 3508 3520 +12
- Partials 1353 1362 +9 ☔ View full report in Codecov by Sentry. |
andy-slac
approved these changes
Oct 11, 2024
|
|
||
| @abstractmethod | ||
| def disassociate( | ||
| self, dataset_type: DatasetType, collection: CollectionRecord, datasets: Iterable[DatasetRef] |
There was a problem hiding this comment.
Do you have to specify DataType here, DatasetRef should already know its DataType?
There was a problem hiding this comment.
This is one of the cases I just didn't want to add to the scope of the ticket, because while you're right, there was just so much of it that it would have been a distraction.
|
|
||
| def clear(self) -> None: | ||
| """Remove everything from the cache.""" | ||
| self._cache = {} | ||
| self._by_name_cache = {} | ||
| self._by_dimensions_cache = {} | ||
| self._full = False |
There was a problem hiding this comment.
Maybe also self._dimensions_full = False?
python/lsst/daf/butler/registry/datasets/byDimensions/_manager.py
Outdated
Show resolved
Hide resolved
TallJimbo
force-pushed
the
tickets/DM-46799
branch
3 times, most recently
from
c578da3
to
152988e
Compare
The lone concrete dataset storage manager class and its intermediate base class have been merged, and many function parameters (all internal, of course) that plumb the SQL dataset ID type through many layers have been dropped.
This is a mostly-mechanical change; for each method: - move a method to the manager class; - give it a dataset type argument, and have it look up the storage object internally; - change calling code (SqlRegistry) to stop looking up the storage object before calling it. The ultimate goal is to get rid of DimensionRecordStorageManager, at least as a public interface. But the steps after this one won't be nearly as mechanical, so it's useful to separate them.
The dataset type cache now holds just the dataset type definition and dataset ID by name, with the SQLAlchemy table objects instead cached by DimensionGroup so they can be shared by multiple dataset types. DatasetRecordStorage has been removed (both the base class and its sole implementation) - its methods had already been moved to DatasetRecordStorageManager, and it no longer works as the opaque thing to put in the cache. Instead there's a new subpackage-private DynamicTables class that is cached by DimensionGroup (this is where the lazy loading of SQLAlchemy table objects now happens), and a module-private _DatasetRecordStorage struct that just combines that with the dataset type and its ID, to make it more convenient to pass all of that around. I also threw in some changes to the insert/import implementations because I started trying to reduce the degree that DatasetRecordStorage was being passed things that were either totally unused or assumed (without checking) have some value. I quickly realized that this problem is ubiquitous (especially with storage classes) and should be a separate ticket, but I've kept what I already did since I think it's a step in the right direction.
Supporting these would be extra complexity I don't think we need.
When the given dataset type differs from the registered dataset type in imports, it's not clear what the ideal behavior is, but the right choice for *this* ticket is clearly to not change that behavior.
TallJimbo
force-pushed
the
tickets/DM-46799
branch
from
152988e
to
79f0f65
Compare
TallJimbo
added a commit
that referenced
this pull request
Oct 17, 2024
Revert "Merge pull request #1095 from lsst/tickets/DM-46799"
TallJimbo
added a commit
that referenced
this pull request
Oct 17, 2024
This reverts commit fd37c27.
TallJimbo
added a commit
that referenced
this pull request
Oct 18, 2024
This reverts commit fd37c27.
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.
Checklist
doc/changesconfigs/old_dimensions