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

fix(hlapi): rework CompressedCiphertextListBuilder #1793

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

tmontaigu
Copy link
Contributor

The hlapi builder target device was selected depending on features (gpu enabled ? gpu : cpu), but if at build time the thread_local key did not match the expected device, an error would be returned.

This is a bit too limiting for users that might want to do some processing on GPU and compression on CPU.

So the Builder is changed to delay, the selection of device used to compress when build is called.
This new design is more flexible for end users, at the cost of a bit more memory copies

  • There should be no API breaking change
  • There is no serialization breaking change as only the builder
    (which is not serializable) has been changed

Copy link
Member

@IceTDrinker IceTDrinker left a comment

Choose a reason for hiding this comment

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

Only one question, otherwise this looks good, though it shows we need to upgrade primitives to use iterators over some types rather than force the WhateverList construct inherited from the previous iteration of the lib

tfhe/src/high_level_api/compressed_ciphertext_list.rs Outdated Show resolved Hide resolved
@tmontaigu
Copy link
Contributor Author

Only one question, otherwise this looks good, though it shows we need to upgrade primitives to use iterators over some types rather than force the WhateverList construct inherited from the previous iteration of the lib

Yeah I sort of considered to do it here, but for this kind of things, it generally we want to iterate twice: once to get the number of lwe to preallocate the glwe_list, and a second time so populate the glwes

@IceTDrinker
Copy link
Member

Only one question, otherwise this looks good, though it shows we need to upgrade primitives to use iterators over some types rather than force the WhateverList construct inherited from the previous iteration of the lib

Yeah I sort of considered to do it here, but for this kind of things, it generally we want to iterate twice: once to get the number of lwe to preallocate the glwe_list, and a second time so populate the glwes

otherwise we need primitives to extend a List instead of manipulating the container, you can enhance ContiguousEntityContainerMut if you want to be able to append

The hlapi builder target device was selected depending on
features (gpu enabled ? gpu : cpu), but if at `build`
time the thread_local key did not match the expected device,
an error would be returned.

This is a bit too limiting for users that might want to do some
processing on GPU and compression on CPU.

So the Builder is changed to delay, the selection of device used
to compress when `build` is called.
This new design is more flexible for end users, at the cost of a
bit more memory copies

* There should be no API breaking change
* There is no serialization breaking change as only the builder
  (which is not serializable) has been changed
@tmontaigu tmontaigu merged commit fcc0378 into main Nov 21, 2024
103 checks passed
@tmontaigu tmontaigu deleted the tm/hlapi-compressed branch November 21, 2024 09:46
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.

3 participants