Json struct alignment

[petrelharp]

Edit: This is now ready. It

• closes json+struct codec should provide a memory alignment guarantee for the binary component #3423, by (a) adding the padding bytes to the C function (tsk_json_struct_metadata_get_blob)
• also closes json+struct codec should check for a length problem with != not < #3424, here

This is pretty straightforward. However, I got confused along the way, basically about "why aren't the C and python code being tested against each other"; and by the end I decided that it was all okay. Keep reading if you want the explanation.

Examining #3306 I see that the python code does not use the C function (tsk_json_struct_metadata_get_blob) at all, so we've got no good current way to test these against each other. Options that I can think of are:

1. remove the C code entirely
2. write an entirely new class of tests that reads in python-produced tree sequence files and then parses them in C and compares to what is expected
3. write a python method that uses the C function
4. not test that the C getter and the python getter do the same thing

However, (2) and (3) are ugly. (2) would break the nice clean one-way C->python arrangment we have. And (3) would leave us with two ways of doing the same thing. This leaves us with (1) or (4).

The motivation for putting the C code in here was that it's very annoying to write and it would help client code a lot to have it available. However, the setter is arguably more useful than the getter (since one does not actually decode the metadata in C), so for (4) it seems we'd still want to additionally write the setter. So, I'm leaning towards (1), out of laziness. We could perhaps document somewhere the example setter/getter code (copied from SLiM and here respectively), to make this easy for other client code, which was the goal.

Edit: see below for the resolution.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.92%. Comparing base (17bacda) to head (728b7e6).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3437   +/-   ##
=======================================
  Coverage   91.92%   91.92%           
=======================================
  Files          37       37           
  Lines       32153    32169   +16     
  Branches     5143     5145    +2     
=======================================
+ Hits        29556    29572   +16     
  Misses       2264     2264           
  Partials      333      333           

Flag	Coverage Δ
C	82.72% <100.00%> (+0.01%)	⬆️
c-python	77.30% <0.00%> (-0.05%)	⬇️
python-tests	96.40% <100.00%> (+<0.01%)	⬆️
python-tests-no-jit	33.21% <0.00%> (-0.01%)	⬇️

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

Components	Coverage Δ
Python API	98.69% <100.00%> (+<0.01%)	⬆️
Python C interface	91.23% <ø> (ø)
C library	88.87% <100.00%> (+<0.01%)	⬆️

🚀 New features to boost your workflow:

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

[petrelharp]

Update: @bhaller tells me that

The problem with having a setter function is that it would require the client to prepare the JSON is a buffer, prepare the binary in a buffer, and then pass pointers to those buffers in to the setter function, which would set up the final aggregate buffer and copy the JSON and binary in.  That is a bad design for performance reasons – it copies all the metadata (which might be quite large), wasting time but even more importantly causing the high-water memory usage for the process to double.  I suppose tskit could provide a setter anyway, for simplicity for clients that don’t care about those performance issues; but SLiM wouldn’t use it, probably.  If having the setter would make your life easier for the testing, I’d have no objection to adding it as long as there is no assertion that clients have to use the setter; the format used by the codec should remain public, and clients should remain free to write their data themselves.

[petrelharp]

After some more discussion with @bhaller:

Why do we want support for JSON+struct? We want to have support for JSON+struct metadata because it is extremely useful. The reason it is extremely useful is that sometimes client code will want to store large amounts of information that doesn't fit in the standard table format for some reason, and this is a very clever (thanks @benjeffery) way to do that, that is way more seamless than other alternatives.

Why have support for it at all in C? Other metadata parsing stuff is not in C at all, for good reason (we don't want to write or import a JSON parser or a struct parser, etcetera). However, the json and struct codecs are pretty straightforward for client C code to do themselves; this codec is annoying enough to set up (special header with magic bytes, etc) that it does make sense to provide a bit of help.

Well then, why don't we have a setter in C, then? I think that #3306 intended to provide a setter, but this fell through the cracks. So, I think it would be a good thing to have for the same reason as in the previous paragraph. However, I can't currently justify the time to actually impmlement and test it. (If we did provide a setter, then (with the note by @bhaller about memory requirements above in mind) we might want to have an API return a pointer to the binary metadata portion, so that the client code could fill it in, rather than passing in the pre-assembled binary portion, thus requiring an extra copy.)

So, I'm leaning towards option (4), "leave things the way they are". This is not perfect because (a) no testing that the C and python do the same thing, and (b) no setter in C; however, what we have is useful and well-tested, and furthermore, if the C and python were in disagreement we'd find out in obvious ways immediately.

[bhaller]

Sounds good, with the caveat that "leave things the way they are" still needs the alignment padding bytes to be added in the python code as we talked about, yes?

[petrelharp]

Okay - this is ready to go. Could @jeromekelleher or @benjeffery take a (hopefully quick) look at this?