Skip to content

fix iter args bug, allow multi-test files #1648

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

Merged
merged 1 commit into from
Apr 2, 2025

Conversation

j2kun
Copy link
Collaborator

@j2kun j2kun commented Apr 2, 2025

Fixes bug with frontend loop generation when there is only one iter arg.

Uses https://pybind11.readthedocs.io/en/stable/advanced/classes.html#module-local-class-bindings to allow multiple compiled functions in the same program.

Better long-term effort would be to cache the pybind11 module in the OpenFHE backend, but as I understand this should still have the local instantiations be interoperable.

Copy link
Collaborator

@asraa asraa left a comment

Choose a reason for hiding this comment

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

thanks! i think the emitter test needs an update?

@j2kun
Copy link
Collaborator Author

j2kun commented Apr 2, 2025

Fixed!

@j2kun j2kun added the pull_ready Indicates whether a PR is ready to pull. The copybara worker will import for internal testing label Apr 2, 2025
@copybara-service copybara-service bot merged commit 55d883d into google:main Apr 2, 2025
8 checks passed
@AlexanderViand-Intel
Copy link
Collaborator

AlexanderViand-Intel commented Apr 8, 2025

Weirdly enough, this works for test files, where each @compile is in its own wrapper function, but you still cannot have two @compile in the same function/scope (e.g., trying to enable the second example in example.py will fail in OpenFHE because of a null-valued secret key).

Doing some mild debugging, it seems like maybe the issue is that somehow the crypto_context we find in the backend for the second function is the crypto_context from the first one?!

@j2kun
Copy link
Collaborator Author

j2kun commented Apr 8, 2025

They should be interoperable, even though they are separate bindings. The long term solution is to have a global cache for the OpenFHE part of the pybindings, but I didn't have time to do that yet.

@AlexanderViand-Intel
Copy link
Collaborator

AlexanderViand-Intel commented Apr 8, 2025

uncommenting the second example in example.py, I get

Traceback (most recent call last):
  File "/home/sdp/code/heir/frontend/example.py", line 33, in <module>
    foo.setup()  # runs keygen/etc
    ^^^^^^^^^^^
  File "/home/sdp/code/heir/frontend/heir/backends/openfhe/backend.py", line 61, in setup
    self.compilation_result.setup_funcs["configure_crypto_context"](
RuntimeError: external/openfhe/src/pke/include/cryptocontext.h:l.371:ValidateKey(): Key is nullptr

However, the first and third example work fine together. I think this is because the first and second generate the *exact same * cryptocontext (there's maybe some uniquing going on within OpenFHE? or maybe pybind relies on == which is defined on cryptocontexts?) because they ask for the same parameters, whereas the third one asks for different parameters (keyswitchcount./evaladdcount differ).

Note: here's why I think this is the case (temp debug output):

If foo is the second @compile:

Using crypto context gen func <built-in method foo__generate_crypto_context of PyCapsule object at 0x7e30cd7146c0> for foo, 
which produces <_heir_func.CryptoContext object at 0x7e30db3994b0>

but if loop_test is the second @compile:

Using crypto context gen func <built-in method loop_test__generate_crypto_context of PyCapsule object at 0x791257f04a80> for loop_test,
which produces <_heir_loop_test.CryptoContext object at 0x791267191a70>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull_ready Indicates whether a PR is ready to pull. The copybara worker will import for internal testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants