Skip to content
New issue

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

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

Already on GitHub? # to your account

openfhe: emit dense elements into a serialized binary file #1501

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

copybara-service[bot]
Copy link
Contributor

@copybara-service copybara-service bot commented Feb 28, 2025

openfhe: emit dense elements into a serialized binary file

To help with #1232

To help with #1232

PiperOrigin-RevId: 731899276
Copy link
Collaborator

@j2kun j2kun left a comment

Choose a reason for hiding this comment

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

Looks good! Relatively minor nits. If you're handling them in followup, LMK. Preemptively approving.

const std::string &weightsFile);

struct Weights {
std::map<std::string, std::vector<float>> floats;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add some comments about the meaning of these fields? At first glance I'd guess the key is an SSA value name, but it takes a bit of searching to confirm. And the values are always dense elements attr values, but is a non-1D shape supported? (I think not, or else the shape would be lost in the archive and I don't see any code to reshape)

#include <vector>

#include "include/cereal/archives/portable_binary.hpp" // from @cereal
#include "include/cereal/cereal.hpp" // from @cereal
Copy link
Collaborator

Choose a reason for hiding this comment

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

This implies that the frontend, if it would like to use this flag for an openfhe backend, requires an include and shared lib for cereal when JIT-compiling, yes? Maybe file an issue for that, though it seems low priority for the moment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to add an e2e openfhe test that runs this program? Otherwise the export/import logic is not tested in this PR, just the codegen for the interface.

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

Successfully merging this pull request may close these issues.

2 participants