Ns/versionize clean by nsarlin-zama · Pull Request #1326 · zama-ai/tfhe-rs

nsarlin-zama · 2024-06-28T14:50:42Z

closes: please link all relevant issues

PR content/description

Various small improvements for the versioning feature. The main ones are:

Versioning for all the serializable types reachable with --features integer,shortint,boolean. The zk are still missing
A custom lint tool that detects types that are serializable but not versioned
the serialize_owned method now takes an owned value. This reduces the number of clone needed for serialization

Check-list:

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)
Relevant issues are marked as resolved/closed, related issues are linked in the description
Check for breaking changes (including serialization changes) and add them to commit message following the conventional commit specification

IceTDrinker

Haven't checked the lint tool for now

IceTDrinker · 2024-07-12T07:55:00Z

+.PHONY: install_tfhe_lints # Install custom tfhe-rs lints
+install_tfhe_lints:
+	(cd utils/cargo-tfhe-lints-inner && cargo install --path .) && \
+	cd utils/cargo-tfhe-lints && cargo install --path .


does it try to reinstall everytime ? My main question here being that if the tool gets updated does the install step try to update the already installed tool ?

Alternatively is the installation mandatory or could it be run as a standalone executable maybe ?

Separate question: does it require a dedicated toolchain as was shown in the tutorial ?

Yes this is reinstalled every times but if its code did not change it is instant.
For the moment I don't think it can be run as a standalone tool because of the way the command line arguments are parsed: let tool_args = std::env::args().skip(2).collect::<Vec<_>>();.
This assumes that the command is invoked with cargo tfhe-lints [args] and will not work with ./target/carg-tfhe-lints [args].

Yes it needs a specific toolchain which is the one used by rustc-tools. The toolchain is specified in the toolchain.txt. Only the cargo-tfhe-lints-inner tool needs this, I should remove the toolchain.txt in cargo-tfhe-lints.

IceTDrinker · 2024-07-12T09:35:34Z

-            integer_key: self.key.versionize_owned(),
+            integer_key: (*self.key).clone().versionize_owned(),


I'm confused here, why the clone ?

self.key is wrapped in an Arc here. So I have to extract the inner type to call versionize_owned. MSRV for the tfhe crate is 1.75 so I cannot use this: https://doc.rust-lang.org/std/sync/struct.Arc.html#method.unwrap_or_clone

However I should at least impl<T: Versionize> Versionize for Arc<T> in tfhe-versionable I think

you are allowed to bump the version and you can do the impl if you want to as well

if you don't need the MSRV bump then don't bump

I added the impl for Arc, this simplifies the code in tfhe since the manual impl is not needed anymore.
I also updated the MSRV.

IceTDrinker

Thanks a lot for your patience and thorough corrections !

For the generated code

BREAKING CHANGE: `versionize_owned` now takes its argument by value.

BREAKING CHANGE: The `Upgrade` trait now requires to specify the Error type as an associated type (similar to `TryFrom`)

cla-bot Bot added the cla-signed label Jun 28, 2024

nsarlin-zama force-pushed the ns/versionize_clean branch from f444677 to 2c36486 Compare June 28, 2024 14:51

nsarlin-zama force-pushed the ns/versionize_clean branch 7 times, most recently from d4d7ea2 to 102815c Compare July 11, 2024 09:15

nsarlin-zama marked this pull request as ready for review July 11, 2024 09:23

nsarlin-zama force-pushed the ns/versionize_clean branch 2 times, most recently from 10cccb0 to c365b14 Compare July 11, 2024 09:40

nsarlin-zama requested a review from IceTDrinker July 11, 2024 09:42

nsarlin-zama force-pushed the ns/versionize_clean branch from c365b14 to 935b754 Compare July 11, 2024 11:31

IceTDrinker reviewed Jul 12, 2024

View reviewed changes

nsarlin-zama force-pushed the ns/versionize_clean branch 8 times, most recently from cf2b141 to 1aced21 Compare July 12, 2024 15:12

IceTDrinker reviewed Jul 12, 2024

View reviewed changes

Comment thread utils/cargo-tfhe-lints/src/main.rs Outdated

IceTDrinker reviewed Jul 12, 2024

View reviewed changes

Comment thread utils/cargo-tfhe-lints/src/main.rs

IceTDrinker reviewed Jul 12, 2024

View reviewed changes

Comment thread utils/cargo-tfhe-lints-inner/Cargo.toml

nsarlin-zama force-pushed the ns/versionize_clean branch 3 times, most recently from 4787b94 to fbda08e Compare July 16, 2024 13:47

IceTDrinker reviewed Jul 16, 2024

View reviewed changes

Comment thread utils/cargo-tfhe-lints/rust-toolchain-link Outdated

nsarlin-zama force-pushed the ns/versionize_clean branch from fbda08e to 208f69f Compare July 16, 2024 14:47

nsarlin-zama mentioned this pull request Jul 16, 2024

Ns/backport 0.7/versionize clean #1367

Merged

4 tasks

IceTDrinker approved these changes Jul 16, 2024

View reviewed changes

zama-bot added the approved label Jul 16, 2024

nsarlin-zama added 12 commits July 17, 2024 09:51

chore(backward): use shallow clone for backward compat tests

66de07a

chore(versionable): add automatically_derived attribute

3e74923

For the generated code

chore(ci): allow '!' for breaking changes in commit messages

6dd23f2

refactor(versionable)!: fix signature of versionize_owned

76521d8

BREAKING CHANGE: `versionize_owned` now takes its argument by value.

feat(versionable): support more tuples

0adda8a

chore(backward): add custom lint to detect missing Versionize implem

3179349

chore(backward): add exceptions to missing versioning lint

e584417

feat(versionable): impl Versionize for Arc

c01e933

feat(all): versionize missing types

ebf35c9

chore(backward): run custom tfhe-rs lints in the ci

3f3fc99

chore(versionable)!: Impl std::error::Error for UnversionizeError

14caba8

BREAKING CHANGE: The `Upgrade` trait now requires to specify the Error type as an associated type (similar to `TryFrom`)

chore(versionable): prepare release 0.2.0

61aeb93

nsarlin-zama force-pushed the ns/versionize_clean branch from 208f69f to 61aeb93 Compare July 17, 2024 07:51

zama-bot removed the approved label Jul 17, 2024

IceTDrinker approved these changes Jul 17, 2024

View reviewed changes

zama-bot added the approved label Jul 17, 2024

nsarlin-zama merged commit 35201b0 into main Jul 17, 2024

nsarlin-zama deleted the ns/versionize_clean branch July 17, 2024 11:44

		integer_key: self.key.versionize_owned(),
		integer_key: (*self.key).clone().versionize_owned(),

Conversation

nsarlin-zama commented Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR content/description

Check-list:

Uh oh!

IceTDrinker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IceTDrinker Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

nsarlin-zama Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IceTDrinker Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

nsarlin-zama Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nsarlin-zama Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

IceTDrinker Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

IceTDrinker Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

nsarlin-zama Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IceTDrinker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nsarlin-zama commented Jun 28, 2024 •

edited

Loading

nsarlin-zama Jul 12, 2024 •

edited

Loading

nsarlin-zama Jul 12, 2024 •

edited

Loading