Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WORK IN PROGRESS] More Reliable Trace Serialization #369

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

georgematheos
Copy link
Contributor

Here's some progress on #129 from the hackathon. Some more debugging work is needed, as well as writing a bit of documentation.

The tests appeared to be passing when I ran them last, but I think there are still bugs to fix which they are not catching.

The 2 main issues I have in mind:

  1. It looked to me like currently, serializing Static DSL traces is not working as intended, and is falling back to constructing DefaultSTs instead of using GenericSTs. (If this is true, the intended mechanism for static trace serialization is untested.)
  2. The current test for whether serializing and deserializing traces works is insufficient, because we attempt to deserialize in the exact same environment as where we serialized. This means that the problems Make traces more reliably (de)serializable #129 describes are not triggered by this test! The test also currently checks for trace equality by testing choicemap equality, which may be insufficient.

@georgematheos
Copy link
Contributor Author

A few more TODO reminders:

  1. Consider renaming GenericST to GenericSerializableTrace (and maybe same for DefaultST).
  2. If I'm going to include DefaultST, I should test it.
  3. I currently have a commented-out implementation of a ChoiceMapST which serializes the choicemap for a trace and unserializes using the choicemap as well. I should think about whether I want to (A) uncomment it and add documentation so users can access it, but not use it, (B) uncomment it and change eg. CustomDeterm... to use it, or (C) just remove it.

@marcoct
Copy link
Collaborator

marcoct commented Feb 13, 2021

@georgematheos Great! When you get a chance, can you also document how a user can ensure their custom generative function trace (and traces of any e.g. DML or SML functions that invoke it) is serializable? E.g. should they implement a custom serialize method for it, or is there another intended way? Or, if that's out of scope, that's fine to document too.

@georgematheos
Copy link
Contributor Author

@marcoct this is now ready for review.

During review, could you please look at:

  • In the documentation, should we list the serialization-related methods as being part of the GFI?
  • Let me know if you have suggestions on the docs in the extending Gen section.

A possible shortcoming of the current testing suite is that it does not simulate the process of saving a trace, quitting a Julia session, and then reloading the trace. It just serializes and deserializes into IOBuffers in RAM -- but it is possible that there are errors relating to the loaded Julia environment changing that this would not catch. Perhaps a better test suite would actually launch a new Julia process to deserialize a trace in.

To confirm that things work, I manually serialized a static DSL trace, then deserialized it in a fresh Julia session; it worked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants