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 muli 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.

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
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