Skip to content

Conversation

AlexanderViand-Intel
Copy link
Contributor

@AlexanderViand-Intel AlexanderViand-Intel commented Jul 29, 2025

Proposed changes

  • Adds data_formats (kylanerace/data-formats with some renaming/cleanup)
  • Adds an OpenFHE tracer which uses the new OpenFHE tracing system
    (See New Tracing Framework AlexanderViand/openfhe-development#22)
  • Adds an example of how to use that tracer (tracing_example)
  • Adds an end-to-end test that pulls together tracing and all p-isa_tools components to show an OpenFHE -> functional_modeler flow with a single cmake target (run_tracing_example)
  • Adds support for multiplication with scalars to the program_mapper
    (needed for the tracing example, which is based on OpenFHE's simple-real-numbers example).

Trying this:

With this PR checked out, run the following from the repo root to configure the project:
mkdir -p build && cmake -B build -S p-isa_tools

Then run cmake --build build --target run_tracing_example
this will create the various output files in build/end-to-end-test.

Note: currently, the functional modeler will fail with an error message about not having partQHatInvModq which is expected, since recent versions of OpenFHE no longer use partQHatInvModq internally but the p-isa_tools still assume so. This has been fixed by re-creating (most of) the metadata, and the simple tracing example now verifies successfully. However, a "real" fix that updates the SDK's kernels to match what OpenFHE 1.3 actually does internally is still needed at some point.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING agreement
  • Current formatting and unit tests / base functionality passes locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Further comments

Some improvements to the core p-isa_tools are needed before we can run a full program (e.g., adapting the new OpenFHE keyswitch approach), but I think this is now at a good point to review.

@faberga
Copy link
Collaborator

faberga commented Jul 29, 2025

Hi @AlexanderViand, few comments:

  • Since this PR touches too many files in its current state, it would be advisable to separate this PR in at least two PRs. One for all the formatting changes, and one for the "data formats" feature. This way there is a separation of concerns.
  • Please check and avoid references to internal repos that are not accessible externaly.
  • I suggest to relocate the "openfhe_tracer" example in a tutorial or test directory, and not part of the main body of the data-formats. The data-formats should remain agnostic to the FHE libraries that may use it.
  • Lastly, I would rename the PR as "Tracing Data formats" so to avoid portraying the SDK as only for OpenFHE.

@AlexanderViand-Intel AlexanderViand-Intel changed the title OpenFHE Tracing -> HERACLES SDK Integration Tracing Data Formats Jul 29, 2025
@AlexanderViand-Intel
Copy link
Contributor Author

AlexanderViand-Intel commented Jul 29, 2025

  • Since this PR touches too many files in its current state, it would be advisable to separate this PR in at least two PRs. One for all the formatting changes, and one for the "data formats" feature. This way there is a separation of concerns.

Done, see #112 :)

Until that's merged, you can see the tracing-specific changes more clearly by viewing only specific (ranges of) commits in the "Files Changed" tab.

@AlexanderViand-Intel
Copy link
Contributor Author

AlexanderViand-Intel commented Jul 30, 2025

I wanted to rebase this on top of the recent pre-commit improvements, but it seems as if the branch protection rules for this repo are (mis?)configured to not allow that. I think @kylanerace is the repo owner/admin and should be able to set it up so that only the main branch (and whatever other branches might be important) have the protection rules applied to them?

PS: While you're in there, could you give write and/or maintain permissions to my main account @AlexanderViand? I switched over 1Source to that account, but the permissions in the IntelLabs org are manually managed and so didn't get updated and now I need to juggle GH accounts.

@kylanerace
Copy link
Collaborator

I wanted to rebase this on top of the recent pre-commit improvements, but it seems as if the branch protection rules for this repo are (mis?)configured to not allow that. I think @kylanerace is the repo owner/admin and should be able to set it up so that only the main branch (and whatever other branches might be important) have the protection rules applied to them?

To confirm, you'd just like to merge #112 into this branch/PR? There shouldn't be any issue doing that (we do this regularly when PR's are out of sync). Checking the branch protection rules for this branch, the only rule is requiring signed commits. Otherwise, you have write access, and should be able to push to it no issue.

PS: While you're in there, could you give write and/or maintain permissions to my main account @AlexanderViand? I switched over 1Source to that account, but the permissions in the IntelLabs org are manually managed and so didn't get updated and now I need to juggle GH accounts.

Your main account isn't in the IntelLabs org I believe though this account (@AlexanderViand-Intel) already is and has the full permissions? I'll need to check with the OSS folks on adding a second account.

@AlexanderViand-Intel
Copy link
Contributor Author

Checking the branch protection rules for this branch, the only rule is requiring signed commits. Otherwise, you have write access, and should be able to push to it no issue.

That's odd, the error I'm getting is specifically mentioning branch protection:

remote: 
remote: - Cannot force-push to this branch
To https://github.com/IntelLabs/encrypted-computing-sdk.git
 ! [remote rejected] aviand/data-formats -> aviand/data-formats (protected branch hook declined)
error: failed to push some refs to 'https://github.com/IntelLabs/encrypted-computing-sdk.git'

(Note that I'm trying to rebase, not merge, sind the latter would give a really messy history)

Your main account isn't in the IntelLabs org I believe though this account (@AlexanderViand-Intel) already is and has the full permissions? I'll need to check with the OSS folks on adding a second account.

Thanks! Doesn't even have to be a second account, switching everything to @AlexanderViand would be fine, too :)

@kylanerace
Copy link
Collaborator

Checking the branch protection rules for this branch, the only rule is requiring signed commits. Otherwise, you have write access, and should be able to push to it no issue.

That's odd, the error I'm getting is specifically mentioning branch protection:

remote: 
remote: - Cannot force-push to this branch
To https://github.com/IntelLabs/encrypted-computing-sdk.git
 ! [remote rejected] aviand/data-formats -> aviand/data-formats (protected branch hook declined)
error: failed to push some refs to 'https://github.com/IntelLabs/encrypted-computing-sdk.git'

(Note that I'm trying to rebase, not merge, sind the latter would give a really messy history)

Ah it's a force push? In general, force pushing was disallowed as per the OSS internal guidelines.
However, for personal branch and meant to clean-up history, I don't see the issue. You should be able to now.

kylanerace and others added 6 commits August 7, 2025 17:07
simplify data_formats dir structure
* update data_formats to new format/pylint/etc

* remove need to run cmake for python protobuf tests

* fix cpplint issues

* fix issues flagged by warnings indicating legitimate issues:
    - comparing against uint -1
    - using clang-only extensions (dynamic-length array)
Note: `run_tracing_example` target works up to expected functional modeler issue `key not found: partQHatInvModq_0_0`
which is due to the fact that partQHatInvModq is no longer present in recent OpenFHE versions.
@AlexanderViand-Intel
Copy link
Contributor Author

Rebased now that the pre-commit improvements have been merged. I think this is a good point to get some reviews, as the next steps aren't really tracing related but about the required improvements for program_mapper/kerngen to handle real OpenFHE v1.3 programs.

@faberga faberga mentioned this pull request Aug 8, 2025
Removing duplicated license note
removing duplicate license note
@kylanerace
Copy link
Collaborator

I was able to get things running with no issues up to the functional_modeler failure you mentioned. Things seem to work as expected up to that point. We will need to further evaluate with all E2E tools/scripts, but probably not before merging this in.

I'd like to pin down exactly what nesting granularity we're tracing at for the default traces (e.g. prior implemented kernels represented "HE-level" ctxt/ptxt operations: add, mul, modsw, relin, rotate, etc.). To that point, generally muli and alike operations were not captured at the kernel level previously, instead being p-isa component operations. Ideally we just agree on what kernels we need to support @christopherngutierrez @AlexanderViand.

Also having the option to break workload further down (e.g. trace out down to the RNS/NTT level), without needing to modify the code, would be significantly useful for workload performance estimations. Just as a future feature-request :)

@AlexanderViand
Copy link
Contributor

I'd like to pin down exactly what nesting granularity we're tracing at for the default traces (e.g. prior implemented kernels represented "HE-level" ctxt/ptxt operations: add, mul, modsw, relin, rotate, etc.). To that point, generally muli and alike operations were not captured at the kernel level previously, instead being p-isa component operations. Ideally we just agree on what kernels we need to support @christopherngutierrez @AlexanderViand.

I think muli is probably just the wrong name here - we could name it mul_scalar or something to make it clear it's a high-level thing? I.e., that we're multiplying a scalar and a ciphertext, not a scalar and a polynomial (iirc, multiplying ctxt c = (a,b) by scalar s just means doing muli(b, s), so there's not much of a "kernel" to it)

@faberga
Copy link
Collaborator

faberga commented Sep 22, 2025

Hi All,

Can we setup a call to discuss this PR, please?
a) I'm not sure about how the dependency on OpenFHE has been made explicit in the main directory structure. Perhaps the tracers/openfhe example should be in a tutorial.
b) Why do we check for OpenMP in the data formats? I would expect the data formats to be thread safe and up to the HE libraries that use the data formats to decide how to use threads.

@AlexanderViand
Copy link
Contributor

AlexanderViand commented Sep 22, 2025

Can we setup a call to discuss this PR, please? a) I'm not sure about how the dependency on OpenFHE has been made explicit in the main directory structure. Perhaps the tracers/openfhe example should be in a tutorial. b) Why do we check for OpenMP in the data formats? I would expect the data formats to be thread safe and up to the HE libraries that use the data formats to decide how to use threads.

I'm not quite sure by what you mean with (a)? There's no need for users to manage the OpenFHE dependency, it's handled automatically via CMake's FetchContent.
@ (b) That's a very good question 😉 I didn't write that code, the PR just shows it as "added" because it came from a different branch, not main) but I had a look, and it seems like data_formats uses #pragma omp parallel for to accelerate various data transformation helpers, which is why it needs OpenMP.

EDIT: I think @kylanerace is OOO at the moment, but once he's back we can try to coordinate a call?

@AlexanderViand
Copy link
Contributor

AlexanderViand commented Sep 22, 2025

I actually got the tracing example working end-to-end just now, by having the tracer re-create some of the metadata that the SDK expects but that OpenFHE 1.3 no longer uses/exposes. The code for this was written with a lot of agent assistance 🤖 so I'd love if one of the metadata experts could take a careful look at the code that generates the metadata: 86bfa87

I also couldn't really figure out what the boot_correction metadata is supposed to be - is that something HERACLES specific?

@fdiasmor fdiasmor self-requested a review September 24, 2025 22:01
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