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

Ns/test data lists #1389

Merged
merged 4 commits into from
Jul 25, 2024
Merged

Ns/test data lists #1389

merged 4 commits into from
Jul 25, 2024

Conversation

nsarlin-zama
Copy link
Contributor

@nsarlin-zama nsarlin-zama commented Jul 19, 2024

PR content/description

Adds tests for heterogeneous lists: Compact (packed or not) and compressed.

This is linked to the following data PR: zama-ai/tfhe-backward-compat-data#14

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

@cla-bot cla-bot bot added the cla-signed label Jul 19, 2024
@IceTDrinker
Copy link
Member

needs a rebase and something is not passing in the build

@nsarlin-zama nsarlin-zama added the data_PR This is a PR that needs to fetch new data for backward compat tests label Jul 19, 2024
@IceTDrinker
Copy link
Member

fast tests are skipped, better get the data workflow in a standalone file

@IceTDrinker
Copy link
Member

you need a tfhe-rs checkout to have the repo present

@IceTDrinker
Copy link
Member

there must be a bug in my code to fetch labels, if need there should be a way to fetch labels of a PR with a github action

@IceTDrinker
Copy link
Member

or maybe something like in the approve_label workflow

    if: ${{ github.event_name == 'pull_request' && contains(fromJSON(env.LABELS), 'approved') }}

change approved for the label you are interested in

@nsarlin-zama nsarlin-zama force-pushed the ns/test_data_lists branch 3 times, most recently from 9048436 to f6541a6 Compare July 19, 2024 13:05
@nsarlin-zama
Copy link
Contributor Author

The workflows should work now:

  • aws_tfhe_backward_compat_tests.yml runs the backward compat tests on a small aws instance, and choses the right branch for data tests if the data_PR label is set
  • data_pr_close.yml automatically merge/close the corresponding PR in the data repo if a PR with data_PR is merged/closed.

These workflows consider that a branch in the data repo exists with the same name as the one with the data_PR label in tfhe-rs.

@nsarlin-zama
Copy link
Contributor Author

For the moment the data_PR label should be added manually when a new versioned data type is added to TFHE-rs.

Comment on lines 83 to 86
- name: Delete the associated branch in the data repo
env:
PR_BRANCH: ${{ github.head_ref || github.ref_name }}
run: |
Copy link
Member

Choose a reason for hiding this comment

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

we can have auto delete enabled in the data repo if needed later on

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.

Looks very nice, few comments on the new workflow which can likely be deduplicated to an extent

.github/workflows/data_pr_close.yml Outdated Show resolved Hide resolved
.github/workflows/data_pr_close.yml Outdated Show resolved Hide resolved
.github/workflows/data_pr_close.yml Outdated Show resolved Hide resolved
@nsarlin-zama nsarlin-zama merged commit 2144ec8 into main Jul 25, 2024
74 checks passed
@nsarlin-zama nsarlin-zama deleted the ns/test_data_lists branch July 25, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved cla-signed data_PR This is a PR that needs to fetch new data for backward compat tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants