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

[serialization] Add support to serialize to memory, and a basic serialization tutorial #7760

Merged
merged 11 commits into from
Sep 28, 2023

Conversation

derek-gerstmann
Copy link
Contributor

No description provided.

src/Deserialization.h Outdated Show resolved Hide resolved
src/Serialization.cpp Show resolved Hide resolved
src/Serialization.cpp Show resolved Hide resolved
src/Serialization.cpp Show resolved Hide resolved
src/Serialization.h Outdated Show resolved Hide resolved
tutorial/lesson_22_serialization.cpp Outdated Show resolved Hide resolved
tutorial/lesson_22_serialization.cpp Outdated Show resolved Hide resolved
@abadams
Copy link
Member

abadams commented Aug 15, 2023

A Julia set is a cool example, but I don't think it belongs in a tutorial about serialization. The tutorial about serialization shouldn't have anything that a reader needs to try to understand in it that's not either introduced in an earlier lesson or directly related to serialization (e.g. the Complex struct). It would be better to use e.g. the simpler blur pipeline from lesson 7, but using an ImageParam instead of the direct Buffer reference so that the tutorial can also handle reconnecting parameters.

@derek-gerstmann
Copy link
Contributor Author

A Julia set is a cool example, but I don't think it belongs in a tutorial about serialization. The tutorial about serialization shouldn't have anything that a reader needs to try to understand in it that's not either introduced in an earlier lesson or directly related to serialization (e.g. the Complex struct). It would be better to use e.g. the simpler blur pipeline from lesson 7, but using an ImageParam instead of the direct Buffer reference so that the tutorial can also handle reconnecting parameters.

Yeah, I went back and forth on this ... I'm happy to change it to a simple blur. My reasoning was that we should have a reasonably complex pipeline to serialize, but I agree that there's too much unrelated material for a tutorial about serialization. I'll change it!

@derek-gerstmann
Copy link
Contributor Author

derek-gerstmann commented Aug 15, 2023

I discovered that the deserialize_pipeline(...) doesn't work on pipelines that have parameters, unless the parameters are populated. So passing an empty params map to the deserialize_pipeline(...) method when there are (eg) ImageParams used in the pipeline triggers an error in deserialize_expr. So we may need to rethink the API to allow introspection or some way of knowing what params exist, etc.

@abadams
Copy link
Member

abadams commented Aug 15, 2023

My understanding was that it was supposed to create new Parameters for you in that case.

@abadams
Copy link
Member

abadams commented Aug 22, 2023

Looks like Param<> acts as a scalar Param with a dynamic type, so for the Internal::Parameter API issue I think the methods should take separate maps for scalar and buffer params, e.g:

Pipeline deserialize_pipeline(const std::string &filename, 
    const std::map<std::string, OutputImageParam> &external_buffer_params, 
    const std::map<std::string, Param<>> &external_scalar_params);

Open to other opinions though. What we discussed in the dev meeting was just making Internal::Parameter public, but that increases our public API surface.

@TH3CHARLie
Copy link
Contributor

That sounds like a good plan, and can be implemented with minimal changes

@steven-johnson
Copy link
Contributor

Can we please introduce aliases for these maps and use them everywhere, since these are unwieldy, eg

using BufferParamMap = std::map<std::string, OutputImageParam>;
using ScalarParamMap = std::map<std::string, Param<>>;

Pipeline deserialize_pipeline(const std::string &filename, 
    const BufferParamMap &external_buffer_params, 
    const ScalarParamMap> &external_scalar_params);

@steven-johnson
Copy link
Contributor

Open to other opinions though. What we discussed in the dev meeting was just making Internal::Parameter public, but that increases our public API surface.

I'm starting to think that might be preferable, assuming we do the relevant API cleanup on Parameter per the discussion

@steven-johnson
Copy link
Contributor

This PR doesn't appear to be very close to ready -- decisions about making Parameter a public API are up in the air, and the bots are largely broken. Should it be converted to draft?

@derek-gerstmann
Copy link
Contributor Author

Yes, I’ll make this to a draft until we sort out the API changes.

Derek Gerstmann added 2 commits September 22, 2023 13:01
Add error messages to deserializer for missing params
Update tutorial
@derek-gerstmann
Copy link
Contributor Author

derek-gerstmann commented Sep 22, 2023

Added issue to track missing ImageParams during deserialization #7862

@derek-gerstmann
Copy link
Contributor Author

Updated to include changes to make parameter map optional for serialize(...) #7849

@derek-gerstmann derek-gerstmann marked this pull request as ready for review September 22, 2023 22:27
@derek-gerstmann
Copy link
Contributor Author

Ready to merge ... we'll address #7862 in another PR.

@derek-gerstmann
Copy link
Contributor Author

@alexreinking @abadams Friendly ping for approval

@derek-gerstmann derek-gerstmann merged commit a24071c into main Sep 28, 2023
3 checks passed
@alexreinking alexreinking deleted the dg/serialize_to_memory branch October 2, 2023 10:34
ardier pushed a commit to ardier/Halide-mutation that referenced this pull request Mar 3, 2024
…lization tutorial (halide#7760)

* Add in-memory buffer serialize/deserialize support.

* Add basic serialization tutorial

* Clang format pass

* Update doc strings to use Doxygen formatted args

* Clear out data buffer during serialization

* Update serialization tutorial to use simple blur example with ImageParam

* Make parameter map optional for serialize halide#7849
Add error messages to deserializer for missing params
Update tutorial

* Clang format pass

---------

Co-authored-by: Derek Gerstmann <dgerstmann@adobe.com>
Co-authored-by: Steven Johnson <srj@google.com>
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.

5 participants