Invalidate caches when Lilypond files are changed - !68 [merged]

[Niols]

https://gitlab.vlanvin.fr/paris-band/dancelor/-/merge_requests/68

This change is the first of a series to make the Dancelor cache persistent across instances of Dancelor (in particular after a victor call). This MR proposes to invalidate existing caches when the underlying Lilypond files are changed. To do so, it adds the Lilypond sources to the key used in the caches hashmaps for the version, set, and book caches. While retrieving the Lilypond source of a version was already exposed, this MR also adds this facility for sets and books, through the addition of the content and lilypond_contents functions to the set and book model signatures respectively. These functions straightforwardly build on top of the existing Version.content.

Note, this MR does not invalidate the cache if the template files are changed, which would be the right behavior to have. It seems that this happens rarely enough to be handled manually at the moment.

[Niols]

If I understand correctly, these new “contents” are not actually the LilyPond contents of the sets/books, but rather a made-up string that concatenates all the raw LilyPond contents of all the versions in the sets/books. Is that correct?

If it is, I think I would prefer a different name for these new functions, as well as some documentation for them.

I suppose their only purpose is to be used as a key in a cache context, so maybe we could even already return a hash and call those functions something like lilypond_content_hash (returning an int). That might be a bit overkill though.

WDYT?

[Niols]

by @R1kM:

That's correct the lilypond_contents for sets and books return all involved Lilypond concatenated into one string. However, I'm not sure I fully see the difference with what is done for versions; contents also returns the raw Lilypond contents there, not including the Lilypond templates.

I'm not particularly attached to the current names, I picked them to remain similar to the version model. Would maybe raw_lilypond_contents be more descriptive? In any case, I'll add some documentation in the mli.

Their only purpose at the moment is indeed to be used in a cache context; I cannot think of a different purpose at the moment, but since you can easily derive a hash from the string (e.g., by using the hashtbl polymorphic hash), I'd be in favor of being future-proof and exposing more information. FWIW, if the only use of these functions is for key caches, it is unclear whether they even need to be in the sets and books models, since they are already implemented "in user-space", i.e. just using already exposed functions from the different models.

[Niols]

I get your point. I don't think the word “raw” is appropriate for something else than the version's contents because it is a value that makes zero sense and isn't actually exploitable. What about lilypond_contents_cache_key or something? Alternatively, the functions for sets and books could return a list of strings, which would make some more sense.

I suppose the functions could indeed just be implemented in the controller modules and that would make quite a bit of sense. No preference there; up to you!

[Niols]

by @R1kM:

Sounds good, I renamed Set.content and Book.lilypond_content into {Set,Book}.lilypond_contents_cache_key. I decided to leave these functions in the models for now, we can always revisit later if needed.

[Niols]

mentioned in commit a34e2debdb4d1d52690f3648dff3fa0771588733 mentioned in commit https://github.com/paris-branch/dancelor/commit/194dfbf97fa707064d841bb1336b419e363f6878