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

chore(ci): run workflows based on file changes #1147

Merged
merged 1 commit into from
May 28, 2024
Merged

Conversation

soonum
Copy link
Contributor

@soonum soonum commented May 14, 2024

This is done to imporve iteration time and feedback for devs.
There is no point to run the full test suite each time. A given development could impact only tfhe-zk-pok for example. In this case only tfhe-zk-pok test would run and thus cutting workflow duration from around 3 hours down to a few minutes.

@soonum soonum added the ci label May 14, 2024
@soonum soonum requested a review from IceTDrinker May 14, 2024 13:30
@soonum soonum self-assigned this May 14, 2024
@cla-bot cla-bot bot added the cla-signed label May 14, 2024
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.

Could we avoid the dispatch and have each workflow perform the file checks ?

Basically a first job run on github ubuntu instance generates the changed files that are relevant for the current workflow, if there are changes then continue in the workflow with the instance set up ?

I don't know if the indirection created by the workflow call is a good idea, is that the pattern concrete used ?

@soonum
Copy link
Contributor Author

soonum commented May 15, 2024

Concrete use the dispatch pattern indeed. Here's the pros and cons of this approach:

  • Pros:
    • file changes check occurs only in place
  • Cons:
    • need to pass around secrets for each target file inducing lot of duplication
    • need to be exhaustive in file checks and define dependencies chain in conditional expression (if ( core_crypto_tests == true && shortint_tests == true [...] )

Regarding your proposal (one file check in each workflow). I think it's a better approach in our case. We define once and for all the file-change check and we carry only one output in the job. Thus we eliminate the need to define long if expression to know if we run the tests or not.

I'll give a shot to your proposal then.

@soonum soonum force-pushed the dt/ci/test_on_changes branch 19 times, most recently from d4030ae to 33043c7 Compare May 17, 2024 08:34
@soonum soonum marked this pull request as ready for review May 17, 2024 08:37
@soonum soonum requested a review from IceTDrinker May 17, 2024 08:37
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.

I think there is just a tiny bit more work to do to manage file dependency chains, that could be detected in the file chnange step

basically we change core, then we want to re run everything that depends on it

csprng likely means core needs to re run etc.

Copy link
Contributor

@mayeul-zama mayeul-zama left a comment

Choose a reason for hiding this comment

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

Thanks!

@IceTDrinker
Copy link
Member

looks like we started instances but then ran no tests https://github.com/zama-ai/tfhe-rs/actions/runs/9254705023/job/25457030476?pr=1147 ?

@soonum

@IceTDrinker
Copy link
Member

probably a check missing to not start instances at all if no file changed

@soonum
Copy link
Contributor Author

soonum commented May 27, 2024

probably a check missing to not start instances at all if no file changed

You're right. Have to fix this.

This is done to imporve iteration time and feedback for devs.
There is no point to run the full test suite each time. A given
development could impact only tfhe-zk-pok for example. In this
case only tfhe-zk-pok test would run and thus cutting workflow
duration from around 3 hours down to a few minutes.
@soonum soonum merged commit 201e385 into main May 28, 2024
37 checks passed
@soonum soonum deleted the dt/ci/test_on_changes branch May 28, 2024 07:07
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.

None yet

4 participants