metadata access counter; closes #3472

[petrelharp]

As lengthily discussed in #3472, if top-level metadata is large then doing ts.metadata a 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 does TreeSequence.metadata or TableCollection.metadata 20 times then they get a Warning suggesting they do something else. The warning only occurs once.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.68%. Comparing base (12b196a) to head (78d5026).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3475      +/-   ##
==========================================
+ Coverage   91.67%   91.68%   +0.01%     
==========================================
  Files          38       38              
  Lines       32188    32232      +44     
  Branches     5151     5153       +2     
==========================================
+ Hits        29509    29553      +44     
  Misses       2346     2346              
  Partials      333      333              

Flag	Coverage Δ
C	82.23% <ø> (ø)
c-python	77.56% <100.00%> (+0.01%)	⬆️
python-tests	96.44% <100.00%> (+0.01%)	⬆️
python-tests-no-jit	33.30% <62.50%> (+0.10%)	⬆️

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

Components	Coverage Δ
Python API	98.73% <100.00%> (+<0.01%)	⬆️
Python C interface	91.25% <100.00%> (+0.01%)	⬆️
C library	91.24% <ø> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[nspope]

Looks fine by me, some minor suggestions

[nspope]

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)?

[nspope]

stray print?

[nspope]

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)

[petrelharp]

Hm: I did it that way because of this bit. Looks like you're right, though, don't need it.

[nspope]

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)

[petrelharp]

Good point about comparisons. I don't think I'm worried about it, though?

Thanks for the input! Will get on it.

[jeromekelleher]

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.

[petrelharp]

I've set METADATA_ACCESS_WARNING_SIZE to 200kb. First I set it to 1Mb, but the tests were annoyingly slow. Here's some anecdata, showing number of bytes and time it takes to run the test that decodes the metadata 25 times:

n             time
1000       0.01s
10000     0.09s
100000   0.92s
1000000 9.2s

So, if we set it to 1Mb, then here we're running 12 tests that would each take 9s (in parallel, but still). However, if 200Kb is too small then we should up that. However, I think we're taking 2s per decode, so we're not warning until the user has spent 40s decoding that could be cut to 2s, so I think this is okay.

Note that I'm using the json+struct codec, but using just json is no faster.

Still TODO: documentation.

[petrelharp]

Docs done. This should be ready to go.

[petrelharp]

Yep - I've had a careful read through, and I think we're good.

[petrelharp]

Here's screenshots of the docs built locally, for convenience:

[nspope]

Docs look good to me, a couple minor nits

[nspope]

Suggested change

	This behavior can be changed, using
	This behavior can be changed, by setting

[jeromekelleher]

I think this is a good approach, but the implementation is a bit complicated. Implementing a cheap metadata_size property here is straightforward, and avoids jumping through hoops.

[jeromekelleher]

Then do

if self._ll_tables.metadata_size > METADATA_ACCESS_WARNING_SIZE:
    self._metadata_access_counter += 1

This is more efficient as it avoids creating another copy of large metadata at instantiation time, and it's more straightforward semantically as you can set the thresholds any time you want.

[petrelharp]

Done! It is certainly cleaner. And hah now I see you only suggested adding metadata_size to the low-level code, not the high-level code, but well I did it anyways.

[petrelharp]

Okay, this is I think finally ready to go!

[jeromekelleher]

Happy to merge after the typo is fixed/variable is renamed.

[jeromekelleher]

Sorry, I missed this the last time. I think we should change this to METADATA_ACCESS_WARNING_COUNT for symmetry with METADATA_ACCESS_WARNING_SIZE (they are both thresholds). This is backtracking a bit on what I said before, I know.

In any case, there's a typo so we'll need to change it (double "H" in threshold)

[petrelharp]

sounds good! and I have now learned more about the word "threshold"! also, tangentially, that it looks like merriam-webster has a five-year cyclical list for "word of the day":

[petrelharp]

Done!