Disable annotations when using Legacy Encoding #1035
Conversation
…rs encoding format. When flatfilebuffers encoding is enabled, spk will add an annotation to the spfs runtimes it make. Signed-off-by: David Gilligan-Cook <dcook@imageworks.com>
…wo methods Add digest_encoding method to spfs graph objects. Removed annotation writing from Layer's legacy_encoding, but not its digest_encoding. size_for_legacy_encoding methods remain unchanged. Signed-off-by: David Gilligan-Cook <dcook@imageworks.com>
dcookspi
changed the title
There was a problem hiding this comment.
I'm a little lost on this one, can you talk about the specific snag that you are running into. I might be over thinking things, but it feels there should never be a need to separate these, or if we do it will cause bugs. If the encoding used for digests is not the same as the encoding used for writing to disk, then something like a server interaction will create fail as the stored data might be saved on the other end using flatbuffers and end up with a different digest than what was expected by the hierachy that's being pushed.
|
FYI the current state of the We would like to make changes to the code to work towards not having "legacy" in the names of methods that are still in common use. We think it is a poor design decision for the flatbuffers encoding to call "legacy" methods. Also, any "legacy" methods should capture the behavior at the time of pre-flatbuffers so we can ensure that we continue to maintain compatibility with pre-flatbuffers going forward. In the current code design, if we put more fields into the flatbuffers schema we have to also add new code to the "legacy" methods to match. It should be an error to attempt to serialize to disk an object when using legacy encoding that makes use of fields that were added in the post-flatbuffers world. Footnotes
|
|
I'll change the
|
Co-authored-by: Ryan Bottriell <ryan@bottriell.ca> Signed-off-by: David Gilligan-Cook <dcook@imageworks.com>
…tion data Signed-off-by: David Gilligan-Cook <dcook@imageworks.com>
dcookspi
force-pushed
the
disable-annotations-when-not-using-ffb
branch
from
300a48a to
c36eab4
Compare
dcookspi
added
SPI AOI
I agree with this, and it was my intention, but things got a little muddled with the flatbuffer and extra data stuff all happening and going in at the same time. We should have caught this case of new logic going into legacy methods during the merge but it slipped through... In any case, this change feels like where we want things to be |
| pub(super) fn legacy_encode(&self, writer: &mut impl std::io::Write) -> Result<()> { | ||
| // Legacy encoded layers do not support writing annotations | ||
| if !self.annotations().is_empty() { | ||
| return Err(Error::String( |
There was a problem hiding this comment.
Although we do want this to error in this situation, the test suite now fails on a number of tests if the default encoding is switched to legacy (as we have done in our internal build). For example: test_layer_encoding_annotation_only.
It would be best if we had tests that test both encoding formats but it isn't clear how we would accomplish this in the current code design, since it relies on one of the formats being marked as default.
There was a problem hiding this comment.
Okay, I found HeaderBuilder and am doing some experiments...
This is an attempt to tighten up the legacy encoding and annotations interaction to make annotations only supported when using the flatbuffers setting. We hit a snag when trying to release a new spk build with annotations and the legacy setting. This is an attempt to make things safer and clearer while we transition.
This stops
spkadding annotations whenspfsis not using the flatbuffers. When flatfbuffers encoding is enabled, spk will still add an annotation to thespfsruntimes it makes.It separates the digest and encoding usage of
legacy_encode()methods on spfs graph objects by using two methods:legacy_encode()anddigest_encode(). Other than their name and what calls them, the methods are virtually identical. The exception is theLayerobject'slegacy_encode(), which ignores any annotation data. TheLayer'sdigest_encode()does write annotations data so it is counted in the hashing.Questions:
size_for_legacy_encode()methods. I wasn't sure of the ramifications. The duplication and rename above should make it easier to remove the legacy_* code in future. But what, if anything, to do with this one?