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

Issue error when serializing ANN models without MLPACK_ENABLE_ANN_SERIALIZATION #3451

Merged
merged 6 commits into from Apr 14, 2023

Conversation

rcurtin
Copy link
Member

@rcurtin rcurtin commented Mar 24, 2023

This is a fix for @yurivict's suggestion in #3449.

If MLPACK_ENABLE_ANN_SERIALIZATION is not defined, then we issue a runtime exception. I tried to make it a static assertion only, so the failure was at compile time, but I couldn't quickly find a way to make the compilation fail only when FFN::serialize() was called (or RNN::serialize()). I think the runtime error is fine though.

I left an "escape mechanism": if the user defines MLPACK_ANN_IGNORE_SERIALIZATION_WARNING, then the exception is not thrown. This is in case a user wants to specifically instantiate serialization for only certain layers, which would be done with custom CEREAL_REGISTER_TYPE() calls.

Copy link
Member

@zoq zoq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is causing some issues with the test cases.

@rcurtin
Copy link
Member Author

rcurtin commented Mar 31, 2023

I think it actually has to do with cotire and precompiled headers. When compiling simple test programs by hand with and without MLPACK_ENABLE_ANN_SERIALIZATION, everything seems to work as expected. I meant to dig into it today, but instead spent my time debugging sorting on the GPU... maybe tomorrow? :)

@rcurtin
Copy link
Member Author

rcurtin commented Apr 12, 2023

Ok, got it figured out now. The fix was relatively straightforward and had to do with cotire, like I thought.

Copy link
Member

@zoq zoq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@conradsnicta conradsnicta merged commit fd7ca06 into mlpack:master Apr 14, 2023
18 of 19 checks passed
@rcurtin rcurtin mentioned this pull request Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants