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.

Sign up for GitHub

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

Jump to bottom

chore(hlapi): stabilize FheTypes #1786

Merged
merged 1 commit into from
Dec 13, 2024
Merged

chore(hlapi): stabilize FheTypes #1786

merged 1 commit into from
Dec 13, 2024

Conversation

mayeul-zama
Copy link
Contributor

No description provided.

@cla-bot cla-bot bot added the cla-signed label Nov 15, 2024
@mayeul-zama mayeul-zama marked this pull request as draft November 15, 2024 09:37
IceTDrinker
IceTDrinker reviewed Nov 15, 2024
View reviewed changes
tfhe/src/c_api/high_level_api/mod.rs Outdated
Comment on lines 25 to 32
Type_FheBool,
Type_FheBool = 0,
Type_FheUint2,
Type_FheUint4,
Type_FheUint4 = 1,
Type_FheUint6,
Type_FheUint8,
Type_FheUint8 = 2,
Type_FheUint10,
Type_FheUint12,
Type_FheUint14,
Copy link
Member

Choose a reason for hiding this comment

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

We need all variants to have a value otherwise we'll have an issue later on, I'll check with BC for that and the fact we have more types than them

Copy link
Member

Choose a reason for hiding this comment

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

we probably need a test (with a macro) to check that all values match between the C Enum and the HL one

IceTDrinker
IceTDrinker requested changes Nov 15, 2024
View reviewed changes
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.

Wait why is this the C API enum being updated ?

@mayeul-zama mayeul-zama force-pushed the mz/stabilize_fhe_types branch 2 times, most recently from cdb6d94 to de4a613 Compare November 25, 2024 17:05
IceTDrinker
IceTDrinker reviewed Nov 26, 2024
View reviewed changes
tfhe/src/high_level_api/mod.rs Outdated
Comment on lines 131 to 134
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
pub enum FheTypes {
Copy link
Member

Choose a reason for hiding this comment

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

this needs to be marked #[repr(i32)] and all the types need to have a value associated, then we'll check with BC it's fine with them

@mayeul-zama mayeul-zama force-pushed the mz/stabilize_fhe_types branch from de4a613 to 08752cd Compare November 27, 2024 17:18
IceTDrinker
IceTDrinker reviewed Dec 2, 2024
View reviewed changes
tfhe/src/c_api/high_level_api/mod.rs Outdated
Comment on lines 42 to 44
Type_FheInt2,
Type_FheInt4,
Type_FheInt6,
Copy link
Member

Choose a reason for hiding this comment

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

still missing values here

@mayeul-zama mayeul-zama force-pushed the mz/stabilize_fhe_types branch from 08752cd to 725438b Compare December 13, 2024 10:37
IceTDrinker
IceTDrinker approved these changes Dec 13, 2024
View reviewed changes
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.

To avoid blocking while I'm away I'm approving, does not mean this should be merged as is

IceTDrinker
IceTDrinker reviewed Dec 13, 2024
View reviewed changes
tfhe/src/high_level_api/mod.rs Outdated
@@ -129,35 +130,37 @@ pub enum Device {
}

#[derive(Copy, Clone, PartialEq, Eq, Debug)]
#[repr(i32)]
#[derive(EnumIter)]
Copy link
Member

Choose a reason for hiding this comment

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

derive only in #[cfg(test)] maybe ? avoids having another mandatory dependency

so #[cfg_attr(test, derive(EnumIter))] I believe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@IceTDrinker
Copy link
Member

as I approved there was this crash that looks related to the new dependency you added https://github.com/zama-ai/tfhe-rs/actions/runs/12315454529/job/34373751603

@mayeul-zama mayeul-zama force-pushed the mz/stabilize_fhe_types branch from 725438b to 408f82c Compare December 13, 2024 13:20
@zama-bot zama-bot removed the approved label Dec 13, 2024
@mayeul-zama mayeul-zama force-pushed the mz/stabilize_fhe_types branch from 408f82c to bf97bb6 Compare December 13, 2024 13:32
@mayeul-zama mayeul-zama marked this pull request as ready for review December 13, 2024 13:38
@mayeul-zama mayeul-zama force-pushed the mz/stabilize_fhe_types branch from bf97bb6 to 3513931 Compare December 13, 2024 14:43
IceTDrinker
IceTDrinker approved these changes Dec 13, 2024
View reviewed changes
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.

This time I'm good with this :)

as long as all is green !

@mayeul-zama mayeul-zama merged commit b1ce34f into main Dec 13, 2024
105 of 108 checks passed
@mayeul-zama mayeul-zama deleted the mz/stabilize_fhe_types branch December 13, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IceTDrinker IceTDrinker IceTDrinker approved these changes

Assignees
No one assigned
Labels
approved cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mayeul-zama @IceTDrinker @zama-bot