reflection_table std::string messagepack writing is completely broken

[ndevenish]

It supports writing of std::string arrays

dials/array_family/reflection_table_msgpack_adapter.h

Lines 665 to 666 in e129e6c

	} else if (name == "std::string") {
	v = extract<std::string>(o.via.array.ptr[1]);

but does so in a broken way; It uses the same "ref/versa" writing as all the standard objects:

dials/array_family/reflection_table_msgpack_adapter.h

Lines 171 to 173 in e129e6c

	std::size_t binary_size = num_elements * element_size;
	o.pack_bin(binary_size);
	o.pack_bin_body(reinterpret_cast<const char*>(&v[0]), binary_size);

This means that it writes to the messagepack file, an area of memory n_items * sizeof(std::string) starting from the &const_ref<std::string>[0]; which is not how std::strings work; e.g. this will definitely contain the first string (but not read it), and if you are lucky this will contain some of the subsequent strings, but basically you are reading random memory.

I don't know if this functionality was ever used for writing strings in the old .pickle format, but it's broken here and we've obviously not used it. Are there any examples of us wanting/needing to write string columns in the reflection tables?

If not, my suggestion is that we just drop the versa<std::string> support.

[phyy-nx]

We use strings in reflection tables in cctbx.xfel.merge:
https://github.com/cctbx/cctbx_project/blob/master/xfel/merging/application/input/file_loader.py#L121
https://github.com/cctbx/cctbx_project/blob/master/xfel/merging/application/input/file_loader.py#L146

This is mostly done in memory, but when we write the data back to disc, we currently have an old as_pickle call. Oddly enough, it's on my list to swap that out to an as_file call to start using msgpack, but I hadn't tested it yet.

[ndevenish]

I think it's relatively easy to fix, just have a custom packer (like Shoeboxes) do. It's good to have an example of it being used, it means it's worth fixing vs throwing out.

[ndevenish]

Some re-discussion of this issue happened in cctbx/dxtbx#465

[graeme-winter]

Thanks @graeme-winter for looking at this. Good set of clues. At the moment I'm inclined to look into whether we can do away with the exp_id column in the cctbx.xfel.merge reflection tables, and instead use the usual size_t id column alongside the experiment id<->experiment identifier map. The exp_id column was developed more or less at the same time as the map, and both solved similar problems for tracking experiment identifiers. I'll look into how big of a change set that would be. Wouldn't fix this or dials/dials#1858, but it would solve our merging problem.

@phyy-nx that is probably a better solution in the long game - I was wondering why you would keep strings in a reflection table.

With that in mind: I am tempted to simply remove support for that (in the messagepack file, at least) rather than having the appearance of support. A "fix" for this is possible but annoying.

[graeme-winter]

😱

in trying to remove this I find that the packing of strings is part of how the general packing works -> also means if we touch this it is likely to break everything

Now working out of I can simply unlink...

[stale]

This issue has been automatically marked as stale because it has not had recent activity. The label will be removed automatically if any activity occurs. Thank you for your contributions.