metadata access counter; closes #3472 - #3475
Conversation
c9090a1 to
d061de1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3475 +/- ##
=======================================
Coverage 91.67% 91.68%
=======================================
Files 38 38
Lines 32188 32213 +25
Branches 5151 5157 +6
=======================================
+ Hits 29509 29534 +25
Misses 2346 2346
Partials 333 333
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
nspope
left a comment
nspope
left a comment
There was a problem hiding this comment.
Looks fine by me, some minor suggestions
| def metadata(self): | ||
| if self._metadata_access_counter is not None: | ||
| self._metadata_access_counter += 1 | ||
| if self._metadata_access_counter > 20: |
There was a problem hiding this comment.
Rather than hardcoding 20 in multiple places, how about defining METADATA_WARNING_COUNT in __init__.py alongside the other constants, and importing it for use here (and in the tests)?
| # should warn after 20 times and no more after that | ||
| md = t.metadata | ||
| for _ in range(19): | ||
| print(_, t._metadata_access_counter) |
| def metadata(self): | ||
| if self._metadata_access_counter is not None: | ||
| # can't set things directly on this immutable class | ||
| builtins.object.__setattr__( |
There was a problem hiding this comment.
Do you need builtins here? object exists without importing, so I think you can just do object.__setattr__ here (and elsewhere) and remove the import. (That's done elsewhere, for example with the _initialised member of the same class)
There was a problem hiding this comment.
Hm: I did it that way because of this bit. Looks like you're right, though, don't need it.
|
Here's one thing which isn't wrong per se but is a bit messy: checking equality may raise the counters of the involved tree sequences/tables, to a degree that varies depending on what's compared. For example, import msprime
import tskit
ts = msprime.sim_ancestry(3, sequence_length=1, random_seed=1024)
tables = ts.dump_tables()
tables.metadata_schema = tskit.MetadataSchema.permissive_json()
tables.metadata = {"a": "bcde", "x": [1, 2, 3, 4]}
ts = tables.tree_sequence()
# mutable vs mutable, same metadata (+0, +0)
t1, t2 = ts.dump_tables(), ts.dump_tables()
t1.assert_equals(t2)
print(t1._metadata_access_counter, t2._metadata_access_counter) # (0, 0)
# mutable vs mutable, diff metadata (+2, +2)
t3, t4 = ts.dump_tables(), ts.dump_tables()
t4.metadata = {"a": "zzz"}
try:
t3.assert_equals(t4)
except AssertionError:
pass
print(t3._metadata_access_counter, t4._metadata_access_counter) # (2, 2)
# immutable vs immutable, same metadata (+1, +1)
t5, t6 = ts.tables, ts.dump_tables().tree_sequence().tables
t5.assert_equals(t6)
print(t5._metadata_access_counter, t6._metadata_access_counter) # (1, 1)I'm not sure there's an easy way to avoid this though (and it's just cosmetic, would just add some unavoidable noise when comparing things in a loop for instance) |
|
Good point about comparisons. I don't think I'm worried about it, though? Thanks for the input! Will get on it. |
jeromekelleher
left a comment
jeromekelleher
left a comment
There was a problem hiding this comment.
I agree this is a good least-bad option here given the dependence on the return value being a mutable copy in downstream code. I think we should make the interface "official" though and make it flexible, as there will be cases where this is really annoying and we want to turn it off.
| ] | ||
|
|
||
| #: Threshhold for warning about multiple top-level metadata use | ||
| METADATA_WARNING_COUNT = 20 |
There was a problem hiding this comment.
As this is part of the public API, I think we should give it a more descriptive name and document how to disable it, like
#: Threshhold for warning about multiple top-level metadata use. To disable, set to a large number
# e.g. ``tskit.METADATA_ACCESS_WARNING_THRESHOLD = 2**32``.
METADATA_ACCESS_WARNING_THRESHOLD = 20
We should add a short description in the docs somewhere also so that it's findable.
|
|
||
| def test_warns_tree_sequence(self): | ||
| t = self.get_example().tree_sequence() | ||
| self.check_warns(t) |
There was a problem hiding this comment.
Don't we need test_warnts_immutable_tables also?
Like
ts = self.get_example().tree_sequence()
self.check_warns(ts.tables)
Also we should test that setting the module variable works so that people can control this. There'll be pytest functionality for this (i.e., setting the module var temporarily)
3 participants
As lengthily discussed in #3472, if top-level metadata is large then doing
ts.metadataa lot of times can be verrrrry slow, because of both the decoding and making a copy of the data (in roughly equal measure). Seeing no straightforward way around this, I'm proposing this stopgap: if someone doesTreeSequence.metadataorTableCollection.metadata20 times then they get a Warning suggesting they do something else. The warning only occurs once.