DM-15424: add limited Registry concept #85

TallJimbo · 2018-09-07T22:38:47Z

Also includes some cleanups to existing code and configuration; recommend reviewing commit-by-commit.

python/lsst/daf/butler/core/registry.py

timj

This looks okay to me. Very minor comments.

timj · 2018-09-12T18:09:17Z

python/lsst/daf/butler/core/dataUnit.py

@@ -264,12 +264,11 @@ def fromConfig(cls, config):
        Parameters
        ----------
        config : `SchemaConfig`


Not a SchemaConfig

timj · 2018-09-12T18:17:40Z

python/lsst/daf/butler/core/schema.py

-        All created tables.
-    views : `dict`
-        All created views.
+        A mapping from table or view name to the associated SQLAlchemy object.


I hadn't really noticed before that Schema requires SQLAlchemy even if Registry is implemented in non-SQL manner.

Yeah, very much so. In fact, while doing some work on DM-15210 it occurred to me that @pschella may have based some of the structure of schema.yaml on what SQLAlchemy wants - I've always found our description of foreign keys hard to parse, for instance, but it turns out they're quite convenient to pass to SQLAlchemy. Anyhow, this bothers me too, but I'm not sure what to do about it and this ticket definitely isn't the place for it.

Schema is indeed quite close to SQLAlchemy, but it doesn't have to actually be required. Overall you need some way for schema.tables['name'] to give you back something that makes sense to use as a table representation. But it doesn't have to be a SQLAlchemy.Table object per-se.

The reason why I deliberately stayed pretty close to a one-to-one mapping between the YAML description and SQLAlchemy's object representation was that I expected us to have to use more complicated schema properties at some later stage. I thus assumed SQLAlchemy did what it did for a reason and it would make our lives easier in matching its expressiveness by having a one-to-one mapping as opposed to something that is perhaps a bit easier to read for the simple cases but requires ugly hacks for the complex ones. This expectation may since have been proven wrong of course ;)

python/lsst/daf/butler/core/registry.py

timj · 2018-09-12T18:32:28Z

python/lsst/daf/butler/core/registry.py

@@ -168,7 +189,12 @@ def transaction(self):

    @property
    def pixelization(self):
-        """Object that interprets SkyPix DataUnit values (`sphgeom.Pixelization`)."""
+        """Object that interprets SkyPix DataUnit values (`sphgeom.Pixelization`).


I think this has to be lsst.sphgeom.Pixelization.

timj · 2018-09-12T18:36:32Z

python/lsst/daf/butler/core/registry.py

-        """Object that interprets SkyPix DataUnit values (`sphgeom.Pixelization`)."""
+        """Object that interprets SkyPix DataUnit values (`sphgeom.Pixelization`).
+
+        ``None`` for limited registries.


I think single back ticks work for None (not that anyone will really be clicking on it to find out what it means).

timj · 2018-09-12T18:39:13Z

python/lsst/daf/butler/core/registry.py

+        Raises
+        ------
+        NotImplementedError
+            If ``self.limited`` is ``True``.


There must be a way to make this link to the classes limited property in the documentation. @jonathansick ?

I'd try

If `limited` is `True`.

If that doesn't work, I'd do

If `RegistryConfig.limited` is `True`.

Also, the recommended way of writing the sentence is:

Raised if `RegistryConfig.limited` is `True`.

https://developer.lsst.io/python/numpydoc.html#py-docstring-raises

Looks like it works if I just say

If `limited` is `True`.

timj · 2018-09-12T18:42:09Z

python/lsst/daf/butler/core/schema.py

@@ -44,6 +45,9 @@ class Schema:
    config : `SchemaConfig` or `str`, optional
        Load configuration. Defaults will be read if ``config`` is not
        a `SchemaConfig`.
+    limited : `bool`
+        If True, ignore tables, views, and associated foreign keys whose


Sometimes True is back ticked, here it is not. Be consistent.

It should always be back ticked in this context.

timj · 2018-09-12T18:42:32Z

python/lsst/daf/butler/core/schema.py

@@ -19,6 +19,7 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.

+


Did we need this new line?

Accident; removed.

timj · 2018-09-12T18:43:04Z

python/lsst/daf/butler/core/schema.py

@@ -44,6 +45,9 @@ class Schema:
    config : `SchemaConfig` or `str`, optional
        Load configuration. Defaults will be read if ``config`` is not
        a `SchemaConfig`.
+    limited : `bool`
+        If True, ignore tables, views, and associated foreign keys whose
+        config descriptions include a 'limited' key set to `False`.


I think the quote character is wrong here.

timj · 2018-09-12T18:44:49Z

python/lsst/daf/butler/core/schema.py

+    ----------
+    limited : `bool`
+        If True, ignore tables, views, and associated foreign keys whose
+        config descriptions include a 'limited' key set to `False`.


Backtick the True (False already is) and fix quote char for limited.

"Registry" here just means "in-memory representation", and probably should have a new name itself. In any case, it's a poor name for the on-disk representation.

...which means other objects that need a DataUnitRegistry shouldn't rely on it to provide one, since it doesn't use it itself.

If basically all of the classes that use Schema use an attribute, it shouldn't start with an underscore.

The signatures and behavior of these will probably change when we get around to implementing them, and they're just cruft right now. And we can always find the old versions in git.

This *probably* doesn't matter, given that Registries are usually constructed via Registry.fromConfig anyway, but this is a bit safer and more consistent with the existing base class logic.

Return None (as we were doing before) messes up other methods provided by UserDict inheritance, like get().

This will permit the same tests to be run on differently-constructed Registry instances.

Registries configured with limited=True do not have any DataUnit metadata or join tables (or the foreign keys from Dataset to those tables). That disables a few operations, but makes it unnecessary to populate those tables just to do simple get and put Butler operations.

TallJimbo · 2018-09-12T20:54:25Z

One more commit: a global replace to do single- instead of double-backticks for True, False, and None.

Rename DataUnit configuration node entries.

8f75405

timj reviewed Sep 8, 2018

View reviewed changes

python/lsst/daf/butler/core/registry.py Outdated Show resolved Hide resolved

TallJimbo force-pushed the tickets/DM-15424 branch from dca2dbf to f0ccae0 Compare September 12, 2018 17:06

timj approved these changes Sep 12, 2018

View reviewed changes

TallJimbo added 9 commits September 12, 2018 16:12

Rename DataUnitRegistryConfig->DataUnitConfig.

306c03e

"Registry" here just means "in-memory representation", and probably should have a new name itself. In any case, it's a poor name for the on-disk representation.

Schema no longer needs a DataUnitRegistry.

60241e2

...which means other objects that need a DataUnitRegistry shouldn't rely on it to provide one, since it doesn't use it itself.

Fix capitalization and whitespace errors in schema.yaml.

b97ef27

Schema doesn't need to hold on to a SchemaBuilder after construction.

a075fe6

Rework Schema public attributes.

dc0fc59

If basically all of the classes that use Schema use an attribute, it shouldn't start with an underscore.

Remove completely unimplemented Registry methods.

e768e79

The signatures and behavior of these will probably change when we get around to implementing them, and they're just cruft right now. And we can always find the old versions in git.

Concrete Registries should coerce their configs before using them.

913c124

This *probably* doesn't matter, given that Registries are usually constructed via Registry.fromConfig anyway, but this is a bit safer and more consistent with the existing base class logic.

Config should raise KeyError when key is not found.

505fede

Return None (as we were doing before) messes up other methods provided by UserDict inheritance, like get().

Refactor SqlRegistry tests.

590ebaa

This will permit the same tests to be run on differently-constructed Registry instances.

TallJimbo force-pushed the tickets/DM-15424 branch from f0ccae0 to 69efe31 Compare September 12, 2018 20:12

TallJimbo added 2 commits September 12, 2018 16:52

Use single-backticks for Python builtins in docs.

c863a78

TallJimbo force-pushed the tickets/DM-15424 branch from 69efe31 to c863a78 Compare September 12, 2018 20:53

TallJimbo merged commit 93f650a into master Sep 13, 2018

timj deleted the tickets/DM-15424 branch April 22, 2020 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-15424: add limited Registry concept #85

DM-15424: add limited Registry concept #85

TallJimbo commented Sep 7, 2018

timj left a comment

timj Sep 12, 2018

timj Sep 12, 2018

TallJimbo Sep 12, 2018

pschella Sep 12, 2018

pschella Sep 12, 2018

timj Sep 12, 2018

timj Sep 12, 2018

timj Sep 12, 2018

jonathansick Sep 12, 2018

jonathansick Sep 12, 2018 •

edited

TallJimbo Sep 12, 2018

timj Sep 12, 2018

jonathansick Sep 12, 2018

timj Sep 12, 2018

TallJimbo Sep 12, 2018

timj Sep 12, 2018

timj Sep 12, 2018

TallJimbo commented Sep 12, 2018

		@@ -19,6 +19,7 @@
		# You should have received a copy of the GNU General Public License
		# along with this program. If not, see <http://www.gnu.org/licenses/>.

DM-15424: add limited Registry concept #85

DM-15424: add limited Registry concept #85

Conversation

TallJimbo commented Sep 7, 2018

timj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathansick Sep 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TallJimbo commented Sep 12, 2018

jonathansick Sep 12, 2018 •

edited