Make metadata lazy for table row classes #1261

benjeffery · 2021-03-20T01:58:16Z

We were decoding metadata on every table row access previously, this makes that lazy. This same method will be applied to the container classes that get returned from e.g. ts.node(u) when they are dataclasses.

Note this also gives table row objects a .table attribute (@hyanwong might be interested) This attribute does not get put into the result of dataclasses.asdict or used in the auto dataclasses methods such as __eq__ or __repr__. These auto methods also only see decoded metadata. The dataclasses are still "frozen".

Needs a couple of extra tests and and one optimisation for the null schema case before merging. Also thinking the schema reference should be stored, as otherwise it could be modified before use by the row.

codecov · 2021-03-20T02:24:13Z

Codecov Report

Merging #1261 (020f2d5) into main (1510d2a) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1261      +/-   ##
==========================================
+ Coverage   93.78%   93.82%   +0.03%     
==========================================
  Files          26       26              
  Lines       21991    21995       +4     
  Branches      987      991       +4     
==========================================
+ Hits        20625    20637      +12     
+ Misses       1325     1324       -1     
+ Partials       41       34       -7

Flag	Coverage Δ
c-tests	`92.49% <ø> (ø)`
lwt-tests	`92.97% <ø> (ø)`
python-c-tests	`95.10% <100.00%> (+0.07%)`	⬆️
python-tests	`98.83% <100.00%> (+0.15%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
python/tskit/metadata.py	`98.78% <100.00%> (+0.12%)`	⬆️
python/tskit/tables.py	`99.74% <100.00%> (+0.12%)`	⬆️
python/tskit/trees.py	`97.86% <100.00%> (+0.42%)`	⬆️
python/tskit/vcf.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1510d2a...020f2d5. Read the comment docs.

jeromekelleher

Looks good, see some comments.

jeromekelleher · 2021-03-20T08:16:37Z

python/tskit/tables.py

+    return cls
+
+
+@lazy_metadata


Not obvious here why we need both a decorator and to inherit from LazyMetadata - one or other is easy to follow for a reader, but both is confusing. What's the rationale here?

I guess we should spell it out up here where these classes are defined anyway, need to encode this knowledge somewhere.

I couldn't figure out how to make an extra slot with a class decorator, annoying!

Should we bother with the decorator then? We should be able to implement this in a straighforward inheritancy type way, right?

Manged to do it all in a decorator in the end, can't do inheritance due to intercepting the constructor.

benjeffery · 2021-03-23T08:28:21Z

@jeromekelleher I think this could have a review now. Docs are not building due to a warning I haven't figured out yet, but look good ignoring the warning.

Note that I've removed the deprecated attrs on mutation - these could be added back if you think it is too early.

jeromekelleher

Looks good, but I'm seriously questioning whether making things frozen is worth it. What problem are we trying to solve here?

Having to resort to this __setattr__ stuff to do perfectly reasonable things seems much more likely to result in errors than anything the frozen is trying to prevent. Plus, I think we are likely to break code if we change the semantics to frozen now.

If we're trying to allow for things like:

tables.nodes[0].flags = 0

then we perhaps we could have table rows frozen for now (to allow this in future) but keep the stuff returned by the TreeSequence unfrozen?

jeromekelleher · 2021-03-23T10:25:25Z

python/tests/simplify.py

@@ -218,7 +219,7 @@ def record_edge(self, left, right, parent, child):
        else:
            last = self.edge_buffer[child][-1]
            if last.right == left:
-                last.right = right
+                builtins.object.__setattr__(last, "right", right)


This isn't a pattern I want to encourage people to use. This seems worse than not having Frozen instances.

Yes agreed - I could rejig these instances to not use the ugly way. It only really needs to be used in the custom __init__ and the metadata decorator.

jeromekelleher · 2021-03-23T10:29:09Z

python/tskit/tables.py

+            and np.array_equal(self.location, other.location)
+            and np.array_equal(self.parents, other.parents)
+            and self.metadata == other.metadata
+        )

    def __neq__(self, other):


Two things here:

I think we can get rid of __neq__ now, this is handled automatically in more recent versions of Python

Can we use the dataclass __eq__? If not, let's add a comment saying why, as this will be a recurring question.

(Ah, answering my own question here: we have numpy arrays. Good to add a quick comment here to that effect)

jeromekelleher · 2021-03-23T10:33:31Z

python/tskit/trees.py

+    """
+    metadata: Any
+    """
+    The decoded :ref:`metadata <sec_metadata_definition>`


Well, only decoded if a schema is present, right?

jeromekelleher · 2021-03-23T10:34:27Z

python/tskit/trees.py

+            and np.array_equal(self.mutations, other.mutations)
+        )
+
+    def __neq__(self, other):


Don't need neq I think

python/tskit/trees.py

jeromekelleher · 2021-03-23T10:37:21Z

python/tskit/trees.py

@@ -560,71 +488,95 @@ class Variant(SimpleContainer):
    Modifying the attributes in this class will have **no effect** on the
    underlying tree sequence data.

-    :ivar site: The site object for this variant.
+    :ivar site:


We can get rid of the ivars entirely I think (hope?)

jeromekelleher · 2021-03-23T10:41:06Z

python/tskit/metadata.py

@@ -39,6 +41,8 @@
 import tskit
 import tskit.exceptions as exceptions

+__setattr__ = builtins.object.__setattr__


I wonder about the wisdom of this - don't we then export this into our namespace? If we're going to do something this ugly, the I'd prefer to feel the full brunt of the ugliness each time we use it, just to be reminded of how ugly it is.

I imported it to avoid the double lookup on each row access - I thinking importing it to something called __builtin__object__setattr__ would be ugly enough?

jeromekelleher · 2021-03-23T10:48:01Z

Note that I've removed the deprecated attrs on mutation - these could be added back if you think it is too early.

When were they marked as deprecated? We should update the changelog with details.

benjeffery · 2021-03-23T16:34:17Z

Looks good, but I'm seriously questioning whether making things frozen is worth it. What problem are we trying to solve here?

The idea was to prevent users thinking they could mutate the classes - as it warned about in the old docs.

Having to resort to this __setattr__ stuff to do perfectly reasonable things seems much more likely to result in errors than anything the frozen is trying to prevent. Plus, I think we are likely to break code if we change the semantics to frozen now.

If we're trying to allow for things like:
tables.nodes[0].flags = 0
then we perhaps we could have table rows frozen for now (to allow this in future) but keep the stuff returned by the TreeSequence unfrozen?

Do you mean the other way round? As in TableRow classes are unfrozen to allow for future mutation, and Node et al. are frozen? This seems to make sense to me.

jeromekelleher · 2021-03-23T19:07:24Z

Do you mean the other way round? As in TableRow classes are unfrozen to allow for future mutation, and Node et al. are frozen? This seems to make sense to me.

No - to me the only point in freezing the classes is so that we don't break people's code later when we change the semantics to "push the changes back into the tables". So, let people mutate away on the Node, Edge etc classes - we document that these are copies of the original data, and they can do whatever they like with them. Freezing the classes just for the sake of it, or to somehow prevent people from writing bad code seems ... unpythonic to me.

jeromekelleher · 2021-03-23T19:10:16Z

How about we make the minimal changes to dataclasses first as cleanly as we can with the same semantics as we currently have (ish), and then look at whether we should freeze or not as a second pass?

benjeffery · 2021-03-24T10:48:53Z

@jeromekelleher OK, semantics are now exactly as they were before. Think I have addressed the comments too. Just the docs
to fix, for some reason it isn't finding the Site class.

…ith dataclasses

jeromekelleher

Looks great, seems like a major improvement. One comment on the type annotations.

jeromekelleher · 2021-03-24T12:46:14Z

python/tests/test_highlevel.py

@@ -2991,17 +2971,18 @@ def test_metadata(self):
        (inst,) = self.get_instances(1)
        (inst2,) = self.get_instances(1)
        assert inst == inst2
-        inst._metadata_decoder = lambda m: "different decoder"
+        inst.metadata


What does this statement do?

I thought it was forcing decode of the metadata, but that is done by the equality operator above anyway, removed.

jeromekelleher · 2021-03-24T12:48:48Z

python/tskit/tables.py

 class IndividualTableRow:
    __slots__ = ["flags", "location", "parents", "metadata"]
    flags: int
    location: np.ndarray
    parents: np.ndarray
-    metadata: bytes
+    metadata: Any


We could say metadata : Union[bytes, dict] couldn't we?

Changed to Optional[Union[bytes, dict]]

benjeffery requested a review from jeromekelleher March 20, 2021 01:59

jeromekelleher reviewed Mar 20, 2021

View reviewed changes

jeromekelleher reviewed Mar 23, 2021

View reviewed changes

benjeffery force-pushed the table-row-metadata branch 3 times, most recently from 3a93746 to afc0ec7 Compare March 24, 2021 10:10

Make metadata lazy for table row classes, replace container classes w…

020f2d5

…ith dataclasses

benjeffery force-pushed the table-row-metadata branch from afc0ec7 to 020f2d5 Compare March 24, 2021 11:00

jeromekelleher approved these changes Mar 24, 2021

View reviewed changes

benjeffery mentioned this pull request Mar 24, 2021

"Append row" for table classes #1254

Merged

benjeffery added the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label Mar 24, 2021

mergify bot merged commit ecf9bcb into tskit-dev:main Mar 24, 2021

mergify bot removed the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label Mar 24, 2021

Make metadata lazy for table row classes #1261

Make metadata lazy for table row classes #1261

Uh oh!

Conversation

benjeffery commented Mar 20, 2021

Uh oh!

codecov bot commented Mar 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jeromekelleher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjeffery commented Mar 23, 2021

Uh oh!

jeromekelleher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeromekelleher commented Mar 23, 2021

Uh oh!

benjeffery commented Mar 23, 2021

Uh oh!

jeromekelleher commented Mar 23, 2021

Uh oh!

jeromekelleher commented Mar 23, 2021

Uh oh!

benjeffery commented Mar 24, 2021

Uh oh!

jeromekelleher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Mar 20, 2021 •

edited

Loading