chore(lint): add a lint to generate a json file with all enum, struct… - #3360
Conversation
93a718c to
d6739cd
Compare
|
ℹ️ Backward-compat snapshot base files were updated Only new types/variants/upgrades were added. This is expected when introducing new versioned types. ➕ Additions
|
575e26d to
26d13e3
Compare
nsarlin-zama
left a comment
There was a problem hiding this comment.
some suggestions, but this is a really good job !
@nsarlin-zama partially reviewed 12 files and all commit messages, and made 17 comments.
Reviewable status: 12 of 29 files reviewed, 16 unresolved discussions (waiting on IceTDrinker, soonum, and SouchonTheo).
.linelint.yml line 13 at r1 (raw file):
- coverage - utils/tfhe-lints/**/main.stderr - /**/*.json
maybe should be scoped to the path where the files are generated?
.github/workflows/main_snapshot_change_check.yml line 50 at r1 (raw file):
run: | mismatch=0 for head_file in utils/tfhe-lints/snapshots/head/*_head.json; do
maybe this could be a script in the ci/ folder, would make it easier to maintain on the long term
utils/tfhe-backward-compat-checker/src/main.rs line 31 at r1 (raw file):
} type Registry = BTreeMap<String, EnumSnapshot>;
nitpick, this could be a newtype: struct Registry(BTreeMap<String, EnumSnapshot>). That way you could define true methods on it for load, get_additions, get_modifications,...
utils/tfhe-backward-compat-checker/src/main.rs line 151 at r1 (raw file):
} fn get_additions(old: &Registry, new: &Registry) -> Vec<String> {
I don't know if based on the number of enum we have perf are important here (I guess not), but it could be made faster by doing only one traversal of both registries for additions, modifications and removal.
Since you use a btree map which is ordered (or even you could instead use a sorted vec) you can iterate on both registries at the same time (manually using .next()), and then if old > new, there is a removal, if new > old there is an addition, if new == old you check for modifs.
This is just an idea if perf matters, no need to change anything if it is already fast enough.
utils/tfhe-lints/Makefile.compat line 9 at r1 (raw file):
lint-tfhe = $(call DYLINT_CMD,$(1),$(2)) -p tfhe -- --features=boolean,shortint,integer,strings,zk-pok lint-tfhe-zk-pok = $(call DYLINT_CMD,$(1),$(2)) -p tfhe-zk-pok -- --features=experimental
did you look at other crates that might use this ? Maybe tfhe-csprng ?
utils/tfhe-lints/Makefile.compat line 32 at r1 (raw file):
lint-generate-report:
I think it would be clearer if this target and lint check below were named something like "backward-snapshot-generate", and "backward-snapshot-check"? "link-check" makes it sound like it checks the lints. This is implemented as a lint but it is an implementation detail.
utils/tfhe-lints/README.md line 130 at r1 (raw file):
make -f utils/tfhe-lints/Makefile.compat lint-generate SUFFIX=my_branch
is this lint-generate-report ? Or I'm missing something in the Makefile?
utils/tfhe-lints/lints/Cargo.toml line 5 at r1 (raw file):
version = "0.1.0" description = "Project specific lints for TFHE-rs" edition = "2021"
could be updated to 2024 I guess ?
utils/tfhe-lints/snapshot/Cargo.toml line 5 at r1 (raw file):
version = "0.1.0" description = "Backward-compatibility snapshot lint for TFHE-rs (VersionsDispatch metadata collection)" edition = "2021"
2024
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 18 at r1 (raw file):
#[derive(Serialize, Clone)] pub struct LintVariantMeta {
Why is this prefixed by "Lint" ?
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 92 at r1 (raw file):
/// /// ### Why is this needed? /// Proc macros only see token-level type names (e.g. `ServerKey`) without full paths, causing
I think the first sentence about proc macro is a bit confusing (someone reading this in the future might not know that you first implemented it with a proc macro). I think the next sentence alone is enough
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 109 at r1 (raw file):
/// For structs: hashes `"field_name:field_type;"` for each field. /// For enums: hashes `"variant_name:field_types;"` for each variant. fn compute_struct_hash<'tcx>(
I'd call this compute_type_hash since it handles enum and structs
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 116 at r1 (raw file):
let mut hasher = Sha256::new(); if adt_def.is_enum() {
Some corner cases to check:
- in rust, types can be struct, enum or unions: https://doc.rust-lang.org/reference/items/unions.html. Do you think they are handled correctly? Since you use "all_fields", it might also cover union
- struct can have "unnamed" fields, (this is usually called a tuple). Will those appear in all_fields ? Maybe try defining a tuple, change the type and check that the hash is modified
- similarly, enum variants can also be named or unnamed (basically, enum variants are structs), so it could make sense to handle them like structs.
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 146 at r1 (raw file):
target_str: &str, ) -> String { let ItemKind::Impl(impl_block) = &impl_item.kind else {
This function could directly take the impl_block as parameter to have static guarantees that this can never fail
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 156 at r1 (raw file):
let impl_item = cx.tcx.hir_impl_item(impl_item_id); if impl_item.ident.as_str() != "upgrade" {
could be a const
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 316 at r1 (raw file):
fn handle_impl<'tcx>(&self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { // Check if this impl is for the Upgrade trait let ItemKind::Impl(impl_block) = &item.kind else {
here also you could avoid this match by having the impl_block as parameter
IceTDrinker
left a comment
There was a problem hiding this comment.
Very quick read for now on my end (day has been long), I'll take more time tomorrow to dig in the details
so as I often do lately : current comments are for stuff around the code and don't address the code itself
still as said elsewhere : looking like a very good basis :)
@IceTDrinker partially reviewed 11 files and all commit messages, and made 6 comments.
Reviewable status: 22 of 29 files reviewed, 19 unresolved discussions (waiting on nsarlin-zama, soonum, and SouchonTheo).
utils/tfhe-lints/Makefile.compat line 9 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
did you look at other crates that might use this ? Maybe tfhe-csprng ?
agreed, the list of crates could be found programmatically by scanning the Cargo.toml files and seeing who has tfhe-versionable as a dependency
utils/tfhe-lints/lints/Cargo.toml line 5 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
could be updated to 2024 I guess ?
if it compiles right away : yes, otherwise don't bother in this PR, some patterns don't work properly in 2024 (due to some changes to temporary lifetimes)
can be done in a follow up if it causes compilation issues
Cargo.toml line 25 at r1 (raw file):
"utils/wasm-par-mq", "utils/wasm-par-mq/examples/msm", "utils/wasm-par-mq/web_tests",
what happened here ?
in general I prefer to avoid "unnecessary" updates to some files in a given PR, because it can:
- hide issues
- make the history contain "unrelated" commits
utils/tfhe-lints/Makefile.compat line 11 at r1 (raw file):
lint-tfhe-zk-pok = $(call DYLINT_CMD,$(1),$(2)) -p tfhe-zk-pok -- --features=experimental .PHONY: lint-base lint-head lint-check
I would keep the .PHONY near their targets
makes it easier to not forget
also we use it for help (try: make help), but not sure it would work if this Makefile is included in another one
utils/tfhe-lints/Makefile.compat line 34 at r1 (raw file):
lint-generate-report: cargo run -p tfhe-backward-compat-checker -- diff-report \ --old-dir $(BASE_DIR) --old-suffix $(BASE_FILE) \
nit: looks like a mix of tab/spaces
26d13e3 to
4099e11
Compare
SouchonTheo
left a comment
There was a problem hiding this comment.
Ready to be review again
@SouchonTheo made 14 comments and resolved 6 discussions.
Reviewable status: 22 of 29 files reviewed, 13 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, and soonum).
.linelint.yml line 13 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
maybe should be scoped to the path where the files are generated?
making sense
Cargo.toml line 25 at r1 (raw file):
Previously, IceTDrinker wrote…
what happened here ?
in general I prefer to avoid "unnecessary" updates to some files in a given PR, because it can:
- hide issues
- make the history contain "unrelated" commits
Each time I rebase I have issue
so I did a sort on all entries
it avoid rebase issue but not neccessary
.github/workflows/main_snapshot_change_check.yml line 50 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
maybe this could be a script in the ci/ folder, would make it easier to maintain on the long term
you are absolutely right
utils/tfhe-backward-compat-checker/src/main.rs line 31 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
nitpick, this could be a newtype:
struct Registry(BTreeMap<String, EnumSnapshot>). That way you could define true methods on it for load, get_additions, get_modifications,...
you are right !
utils/tfhe-backward-compat-checker/src/main.rs line 151 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
I don't know if based on the number of enum we have perf are important here (I guess not), but it could be made faster by doing only one traversal of both registries for additions, modifications and removal.
Since you use a btree map which is ordered (or even you could instead use a sorted vec) you can iterate on both registries at the same time (manually using.next()), and then if old > new, there is a removal, if new > old there is an addition, if new == old you check for modifs.This is just an idea if perf matters, no need to change anything if it is already fast enough.
this is already super fast (fast enough for generating a file lol)
but you are right if we can do better I will try to do it !
utils/tfhe-lints/Makefile.compat line 9 at r1 (raw file):
Previously, IceTDrinker wrote…
agreed, the list of crates could be found programmatically by scanning the Cargo.toml files and seeing who has tfhe-versionable as a dependency
good catch will take a look
utils/tfhe-lints/Makefile.compat line 11 at r1 (raw file):
Previously, IceTDrinker wrote…
I would keep the .PHONY near their targets
makes it easier to not forget
also we use it for help (try: make help), but not sure it would work if this Makefile is included in another one
I made an update on the make help at the root
I also made the change requested
utils/tfhe-lints/Makefile.compat line 32 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
I think it would be clearer if this target and lint check below were named something like "backward-snapshot-generate", and "backward-snapshot-check"? "link-check" makes it sound like it checks the lints. This is implemented as a lint but it is an implementation detail.
you are right, I will do the change
utils/tfhe-lints/Makefile.compat line 34 at r1 (raw file):
Previously, IceTDrinker wrote…
nit: looks like a mix of tab/spaces
good catch
utils/tfhe-lints/README.md line 130 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
is this lint-generate-report ? Or I'm missing something in the Makefile?
you are right the md is super outdated
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 116 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
Some corner cases to check:
- in rust, types can be struct, enum or unions: https://doc.rust-lang.org/reference/items/unions.html. Do you think they are handled correctly? Since you use "all_fields", it might also cover union
- struct can have "unnamed" fields, (this is usually called a tuple). Will those appear in all_fields ? Maybe try defining a tuple, change the type and check that the hash is modified
- similarly, enum variants can also be named or unnamed (basically, enum variants are structs), so it could make sense to handle them like structs.
you are right the corner case was the field name that I ignore
I made a fix for it
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 156 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
could be a const
Done.
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 316 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
here also you could avoid this match by having the impl_block as parameter
I did it but I kept item for the owner id
|
Previously, SouchonTheo (Theo Souchon) wrote…
maybe cargo metadata can be waht you are looking for |
4099e11 to
916528b
Compare
nsarlin-zama
left a comment
There was a problem hiding this comment.
@nsarlin-zama partially reviewed 20 files and all commit messages, made 3 comments, and resolved 7 discussions.
Reviewable status: 30 of 31 files reviewed, 7 unresolved discussions (waiting on IceTDrinker, soonum, and SouchonTheo).
utils/tfhe-backward-compat-checker/src/main.rs line 151 at r1 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
this is already super fast (fast enough for generating a file lol)
but you are right if we can do better I will try to do it !
Nice!
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 116 at r1 (raw file):
Nice! Looks like it handles all cases, from the rustc doc:
structs, tuples, and unionss are considered to have a single variant with variant index zero, aka FIRST_VARIANT.`
Maybe you can add a comment about that to explain that it works on struct too.
Did you confirm that you go through all the fields for tuples also? Will field_name be empty in that case?
utils/tfhe-backward-compat-checker/src/main.rs line 104 at r2 (raw file):
eprintln!("Cannot read directory {}: {}", dir.display(), err); eprintln!("Return empty registry"); return Registry(snapshots);
Is it ok to return an empty registry if read_dir fails? Maybe we should propagate the error instead?
916528b to
5c48b21
Compare
|
Previously, IceTDrinker wrote…
in the end it seems this flag allow_additional_enums may not be useful |
SouchonTheo
left a comment
There was a problem hiding this comment.
@SouchonTheo made 14 comments and resolved 2 discussions.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, and soonum).
.github/workflows/backward_compat_snapshot_consistency.yml line 23 at r11 (raw file):
Previously, IceTDrinker wrote…
I hope I did not suggest to check paths for the files, but I think we can always run the sanity checker for snapshots on main, it does not take much time and is a nice safety to have
no it was on purpose on my side
I will remove it if you want
Do you want to remove it in the other CI too ?
scripts/check_snapshot_consistency.sh line 14 at r8 (raw file):
Previously, IceTDrinker wrote…
yep I like that approach 👍
Done.
scripts/check_snapshot_consistency.sh line 1 at r11 (raw file):
Previously, IceTDrinker wrote…
I think you are re-implementing the diff program
example output
diff -q tfhe-rs tfhe-rs-other/ ... truncated Only in tfhe-rs: venv Only in tfhe-rs: web-test-runner user:~/Documents/zama/code$ echo $? 1returns non 0 code on exit, there certainly is a summary mode
so I think you can get rid of this script and just use diff -q
ack
utils/tfhe-backward-compat-checker/Cargo.toml line 8 at r8 (raw file):
Previously, IceTDrinker wrote…
yep prefer workspace deps
yes will do the change
utils/tfhe-backward-compat-checker/src/diff.rs line 34 at r11 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
Only case where this might be legal is if done on a variant introduced and removed before a release.
In the other cases this is extremely dangerous because it means that we will have a version confusion. So technically this may be even worse than what the comment says since it might be possible to deserialize it, just not to the correct type.
so I let it like that ? (that was my understanding)
utils/tfhe-backward-compat-checker/src/main.rs line 106 at r11 (raw file):
Previously, IceTDrinker wrote…
in the end it seems this flag allow_additional_enums may not be useful
removed the flag and keeped the test !
utils/tfhe-lints/common/Cargo.toml line 13 at r8 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
If I remember correctly this comes from an auto-generated file. The commit is updated automatically when we update the toolchain using the
cargo dylintcommand, maybe it's worth checking this still works here
if this work like that no need to worry in the end when we will update it will change accordingly
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 325 at r6 (raw file):
Previously, IceTDrinker wrote…
For my culture what other TyKind are there ?
https://doc.rust-lang.org/nightly/nightly-rustc/rustc_type_ir/ty_kind/enum.TyKind.html
.github/workflows/backward_compat_pr_change_report.yml line 44 at r11 (raw file):
Previously, IceTDrinker wrote…
a tiny bit confused between the ../report.md
and the report.md below ?
I'm generating the report at the root of the ubuntu (because we are in head when we run the command)
utils/tfhe-lints/Makefile line 5 at r11 (raw file):
Previously, IceTDrinker wrote…
curious about how this works ? I'm not a Make guru :)
$($(1)) ->
if cond,then,else
the cond is evaluating this give somehing
no then bc everything is fine
else is an error to told the user to set the vars
utils/tfhe-lints/Makefile line 18 at r11 (raw file):
Previously, IceTDrinker wrote…
not sure if that's the best name
dylint_all ?
since we have a clippy_all in the main Makefile
does it run both the consistency checks and the snapshots ?
dylint_all is not the right name
in fine the run-all-carte is just the setup of the dylint for some rules
here we can run the snapshot or the lint rules or we also can do both
I would not rename it as it's a macro not callable from the outside
utils/tfhe-lints/Makefile line 19 at r11 (raw file):
Previously, IceTDrinker wrote…
I'm going to be annoying, I think it's interesting to have separate targets, when you dev on e.g. CSPRNG you only want to check CSPRNG e.g. and you don't have access to the target :)
you can keep the macros you defined, but having single targets can be useful for fine grained debug/lints
oki
utils/tfhe-lints/Makefile line 47 at r11 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
small nitpick, but I think it's better if all the targets use '_' and not '-' since it's the convention in the other makefile
you are right mb
utils/tfhe-lints/Makefile line 54 at r11 (raw file):
Previously, IceTDrinker wrote…
should this require a HEAD_SNAPSHOT_DIR like you did below ?
for those one no
we want to enforce it in the right place
e983096 to
b20b135
Compare
IceTDrinker
left a comment
There was a problem hiding this comment.
I think we are about to finish here :)
just one thing I would like you to check about the report (I have a doubt around the location it is saved in CI)
maybe you could error out if the file does not exist, because then it means something went wrong in the CI
so maybe you want a -f check instead of a -s
first : -f checks the file exist, then if it's not empty (-s check) you can display things
I think there was something about a comment where it does not convey enough that a variant removal is "a crime"
@IceTDrinker partially reviewed 8 files and all commit messages, made 4 comments, and resolved 7 discussions.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on nsarlin-zama, soonum, and SouchonTheo).
.github/workflows/backward_compat_snapshot_consistency.yml line 23 at r11 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
no it was on purpose on my side
I will remove it if you want
Do you want to remove it in the other CI too ?
we could, though in the case of the PR (for the other workflow) we know that the files changed or not, while merging on main we don't know what got merged before that could issues
utils/tfhe-backward-compat-checker/src/diff.rs line 34 at r11 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
so I let it like that ? (that was my understanding)
I think you may want to put a comment that indicates it's a much worse outcome than "no longer be deserialized" Nicolas is talking about version confusion, which is essentially type confusion (very dangerous for security)
.github/workflows/backward_compat_pr_change_report.yml line 44 at r11 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
I'm generating the report at the root of the ubuntu (because we are in head when we run the command)
since we did a cd we should be in the same dir ? I'm not sure, can you check the latest runs find the report ?
here it feels like (could be wrong) that report.md is one level above, but we are looking for the report in the current dir, so we are not updating the comment at the moment ?
nsarlin-zama
left a comment
There was a problem hiding this comment.
@nsarlin-zama partially reviewed 4 files, made 2 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on IceTDrinker, soonum, and SouchonTheo).
utils/tfhe-backward-compat-checker/src/diff.rs line 34 at r11 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
so I let it like that ? (that was my understanding)
That's what I would do, yes
utils/tfhe-lints/common/Cargo.toml line 13 at r8 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
if this work like that no need to worry in the end when we will update it will change accordingly
I just tested, it looks like it works, but you need to put clippy_utils and dylint_linting in the Cargo.toml above in a [workspace.dependencies] section, and then use { workspace = true } here (and in the other lints)
46ec630 to
1ee149d
Compare
SouchonTheo
left a comment
There was a problem hiding this comment.
yep ! I will do something to be super safe
@SouchonTheo made 5 comments.
Reviewable status: 28 of 35 files reviewed, 8 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, and soonum).
.github/workflows/backward_compat_snapshot_consistency.yml line 23 at r11 (raw file):
Previously, IceTDrinker wrote…
we could, though in the case of the PR (for the other workflow) we know that the files changed or not, while merging on main we don't know what got merged before that could issues
yep
utils/tfhe-backward-compat-checker/src/diff.rs line 34 at r11 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
That's what I would do, yes
ok will add something like you suggest
utils/tfhe-lints/common/Cargo.toml line 13 at r8 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
I just tested, it looks like it works, but you need to put clippy_utils and dylint_linting in the Cargo.toml above in a
[workspace.dependencies]section, and then use{ workspace = true }here (and in the other lints)
done
.github/workflows/backward_compat_pr_change_report.yml line 44 at r11 (raw file):
Previously, IceTDrinker wrote…
since we did a cd we should be in the same dir ? I'm not sure, can you check the latest runs find the report ?
here it feels like (could be wrong) that report.md is one level above, but we are looking for the report in the current dir, so we are not updating the comment at the moment ?
ohhhh
yes you are right I was thinking of the other step not the other one
when I modified with -s and -f I made the fix my bad
IceTDrinker
left a comment
There was a problem hiding this comment.
@IceTDrinker partially reviewed 7 files and all commit messages, made 4 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on soonum and SouchonTheo).
.github/workflows/backward_compat_snapshot_consistency.yml line 23 at r11 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
yep
one thing to keep in mind is if we change the location of the snapshots then the trigger in the other workflow won't work, but it's not a concern currently
.github/workflows/backward_compat_pr_change_report.yml line 44 at r11 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
ohhhh
yes you are right I was thinking of the other step not the other one
when I modified with -s and -f I made the fix my bad
I think the current version is wrong, it says there is no report if -f ../report is true
my thinking reading your current code is something like this :
I think I would write the code as the following :
if [ -f ../report.md ]; then
if [ -s ../report.md ]; then
# size is non zero, we have a report
has_report = true
else
has_report = false
fi
else
echo "::error::report.md was not created — something went wrong"
exit 1
fithe above is pseudo code so not fully correct
one thing is likely to set a variable to ../report.md to avoid repeating the same name every time
we could have a protection rule on that step so that we are sure the step ran properly if needed (I can set this up later)
utils/tfhe-lints/common/Cargo.toml line 14 at r16 (raw file):
[dependencies] clippy_utils = { workspace = true } dylint_linting = "5.0.0"
can use workspace I think ?
.github/workflows/backward_compat_pr_change_report.yml line 20 at r16 (raw file):
jobs: change-report: name: backward_compat_pr_change_report/change-report
since we are going to verify that this step ran successfully later (verify the report is correctly generted), can we have (bpr) added in the name, it's a way we have to find branch proections rules more easily
example we have in the lib:
jobs:
backward-compat-tests:
name: aws_tfhe_backward_compat_tests/backward-compat-tests (bpr)1ee149d to
ecea26d
Compare
SouchonTheo
left a comment
There was a problem hiding this comment.
@SouchonTheo made 4 comments.
Reviewable status: 34 of 35 files reviewed, 8 unresolved discussions (waiting on IceTDrinker and soonum).
.github/workflows/backward_compat_snapshot_consistency.yml line 23 at r11 (raw file):
Previously, IceTDrinker wrote…
one thing to keep in mind is if we change the location of the snapshots then the trigger in the other workflow won't work, but it's not a concern currently
yep ofc
utils/tfhe-lints/common/Cargo.toml line 14 at r16 (raw file):
Previously, IceTDrinker wrote…
can use workspace I think ?
yep did it along the request of Nicolas
.github/workflows/backward_compat_pr_change_report.yml line 44 at r11 (raw file):
Previously, IceTDrinker wrote…
I think the current version is wrong, it says there is no report if -f ../report is true
my thinking reading your current code is something like this :
I think I would write the code as the following :
if [ -f ../report.md ]; then if [ -s ../report.md ]; then # size is non zero, we have a report has_report = true else has_report = false fi else echo "::error::report.md was not created — something went wrong" exit 1 fithe above is pseudo code so not fully correct
one thing is likely to set a variable to ../report.md to avoid repeating the same name every time
we could have a protection rule on that step so that we are sure the step ran properly if needed (I can set this up later)
if [ -s report.md ]; then
echo "has_report=true" >> "$GITHUB_OUTPUT"
elif [ -f report.md ]; then
echo "has_report=false" >> "$GITHUB_OUTPUT"
else
echo "::error::report.md was not created — something went wrong"
exit 1
fidid that before seing your message
.github/workflows/backward_compat_pr_change_report.yml line 20 at r16 (raw file):
Previously, IceTDrinker wrote…
since we are going to verify that this step ran successfully later (verify the report is correctly generted), can we have (bpr) added in the name, it's a way we have to find branch proections rules more easily
example we have in the lib:
jobs: backward-compat-tests: name: aws_tfhe_backward_compat_tests/backward-compat-tests (bpr)
will add it
nsarlin-zama
left a comment
There was a problem hiding this comment.
@nsarlin-zama partially reviewed 6 files and made 3 comments.
Reviewable status: all files reviewed (commit messages unreviewed), 9 unresolved discussions (waiting on IceTDrinker, soonum, and SouchonTheo).
utils/tfhe-lints/common/Cargo.toml line 14 at r16 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
yep did it along the request of Nicolas
but it looks like this one is still using the 5.0.0 and not workspace
.github/workflows/backward_compat_pr_change_report.yml line 44 at r11 (raw file):
Previously, IceTDrinker wrote…
I think the current version is wrong, it says there is no report if -f ../report is true
my thinking reading your current code is something like this :
I think I would write the code as the following :
if [ -f ../report.md ]; then if [ -s ../report.md ]; then # size is non zero, we have a report has_report = true else has_report = false fi else echo "::error::report.md was not created — something went wrong" exit 1 fithe above is pseudo code so not fully correct
one thing is likely to set a variable to ../report.md to avoid repeating the same name every time
we could have a protection rule on that step so that we are sure the step ran properly if needed (I can set this up later)
I think in the current code if you reach the -f it means that -s is false so the file exists but is empty ? so it's correct to set has_report to false ? What I understand:
- file is present and has content: has_report=true
- file is present and empty: has_report=false
- no file: error
utils/tfhe-backward-compat-checker/src/diff.rs line 35 at r16 (raw file):
/// A variant was removed from a versions dispatch enum. /// Existing serialized data referencing this variant can no longer be deserialized /// or even worse a version confusion could happen which lead to dangerous security issue.
"which leads ..." I think ?
IceTDrinker
left a comment
There was a problem hiding this comment.
only missing workspace and we are good to go :)
@IceTDrinker partially reviewed 1 file and all commit messages, made 3 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on nsarlin-zama, soonum, and SouchonTheo).
utils/tfhe-lints/common/Cargo.toml line 14 at r16 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
but it looks like this one is still using the 5.0.0 and not workspace
yes, hasn't been updated here
.github/workflows/backward_compat_pr_change_report.yml line 44 at r11 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
I think in the current code if you reach the -f it means that -s is false so the file exists but is empty ? so it's correct to set has_report to false ? What I understand:
- file is present and has content: has_report=true
- file is present and empty: has_report=false
- no file: error
ah yes my bad, seems it's ok then !
ecea26d to
5608ea5
Compare
SouchonTheo
left a comment
There was a problem hiding this comment.
@SouchonTheo made 1 comment.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, and soonum).
utils/tfhe-lints/common/Cargo.toml line 14 at r16 (raw file):
Previously, IceTDrinker wrote…
yes, hasn't been updated here
ah yes ofc it's just pushed rn
… and upgrade information hashed
5608ea5 to
e1dc9c0
Compare
SouchonTheo
left a comment
There was a problem hiding this comment.
@SouchonTheo made 1 comment.
Reviewable status: 34 of 35 files reviewed, 7 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, and soonum).
utils/tfhe-backward-compat-checker/src/diff.rs line 35 at r16 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
"which leads ..." I think ?
Even did a new version way better !
nsarlin-zama
left a comment
There was a problem hiding this comment.
Huge work, thanks a lot !!
@nsarlin-zama made 2 comments.
Reviewable status: 32 of 35 files reviewed, 7 unresolved discussions (waiting on IceTDrinker, soonum, and SouchonTheo).
utils/tfhe-backward-compat-checker/src/diff.rs line 35 at r16 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
Even did a new version way better !
perfect :)
IceTDrinker
left a comment
There was a problem hiding this comment.
As nsarlin said, huge PR, and will very useful !
Not approving since the CI is already running.
Well done !
@IceTDrinker reviewed 3 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on soonum and SouchonTheo).
IceTDrinker
left a comment
There was a problem hiding this comment.
Actually I need to approve since I'm currently blocking, here goes ! 🚀

PR regarding lints
CI
New CI file to run the checker in the PR
It will return an error if
tfhe-lints
Now tfhe-lints contain 3 crate
(AI was used to wrote some documentation)
Checker
there is also a new crate called checker where the check happen
you have all the rules we need to follow and everything is tested
(AI was used to generate test case)
Makefile
The makefile was updated to only run the right lib
Also a new Makefile arrived
Makefile.compatinside it you have everything you need to generate or check the stuff you need (AI was used to help me create some rules)
This change is