Skip to content
This repository has been archived by the owner on Dec 18, 2023. It is now read-only.

Fix #178: Make MNT{4,6}-753, cp6_782 tests run conditionally #179

Merged
merged 27 commits into from
Sep 25, 2023

Conversation

z-tech
Copy link
Contributor

@z-tech z-tech commented Sep 22, 2023

Make MNT{4,6}-753, cp6_782 tests run conditionally

Fixes #178
Fixes #180

What does this PR do?

  • updates ci.yml so that tests for MNT{4,6}-753 and cp6_782 are run only when either:

    • a) relevant source code changes
    • b) dependencies are updated
  • I sanity checked the following conditions:

    • code was updated
    • code was not updated
    • deps were updated
    • deps were not updated

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests n/a
  • Updated relevant documentation in the code n/a
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md n/a
  • Re-reviewed Files changed in the Github PR explorer ?

@z-tech z-tech added T-test Type: test improvements Do not merge labels Sep 22, 2023
@z-tech z-tech marked this pull request as draft September 22, 2023 13:36
@Pratyush
Copy link
Member

This is a good start! Nice find on the action. However as things stand this is a bit too aggressive, because it doesn't handle when the dependencies change. I think the way to handle that would be to check the current Cargo.lock against the previous Cargo.lock. To perform the comparison we would need to cache the old Cargo.lock.

@Pratyush Pratyush marked this pull request as ready for review September 22, 2023 15:36
@Pratyush

This comment was marked as resolved.

@z-tech

This comment was marked as resolved.

@z-tech z-tech changed the title 178: make ci conditional for mnt4_753, mnt6_753 ticket-178: make long tests conditional Sep 25, 2023
@z-tech z-tech changed the title ticket-178: make long tests conditional Repo organization and CI #178: Make long tests run conditionally Sep 25, 2023
@z-tech z-tech changed the title Repo organization and CI #178: Make long tests run conditionally Fix #178: Make MNT{4,6}-753 tests run conditionally Sep 25, 2023
@z-tech z-tech changed the title Fix #178: Make MNT{4,6}-753 tests run conditionally Fix #178: Make MNT{4,6}-753, cp6_782 tests run conditionally Sep 25, 2023
@z-tech z-tech requested review from weikengchen, mmagician, mmaker and a team and removed request for mmaker, mmagician and weikengchen September 25, 2023 13:25
@z-tech
Copy link
Contributor Author

z-tech commented Sep 25, 2023

Okay sweet I think this does what we want LMK

@Pratyush
Copy link
Member

Looks great! Maybe the only diff would be to update the other cache action invocation (line 52) to v3 also.

~/.cargo/registry
~/.cargo/git
target
key: mnt4_753-deps-updated-${{ runner.os }}-${{ hashFiles('**/Cargo.lock') }}
Copy link
Member

Choose a reason for hiding this comment

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

I think we can avoid the mnt4_753-deps-updated- prefix, as the Cargo.lock file is shared across the entire workspace. So we'd be unnecessarily creating an additional cache entry and possibly causing false cache misses if we use this key. Ditto for the other curves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That last change didn't result in "cache hits" like I expected...

Does cargo generate-lockfile need to be added before the cache invocation at line 53?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Second run seems to work as expected. I'll wait for you to let me know when to merge.

@Pratyush
Copy link
Member

Mostly looks good to me! A minor optimization that we could do would be to simply check to see if the cache exists, instead of downloading it also. See https://github.com/actions/cache#inputs for how to do this. Thanks for the great work!

@Pratyush Pratyush merged commit 0a64024 into master Sep 25, 2023
33 checks passed
@Pratyush Pratyush deleted the curves-ticket-178 branch September 25, 2023 18:28
@z-tech
Copy link
Contributor Author

z-tech commented Sep 25, 2023

Mostly looks good to me! A minor optimization that we could do would be to simply check to see if the cache exists, instead of downloading it also. See https://github.com/actions/cache#inputs for how to do this. Thanks for the great work!

Sweet! Thanks for the help! I made a little note: arkworks-rs/algebra#720

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-test Type: test improvements
Projects
Development

Successfully merging this pull request may close these issues.

Run cp6_782 conditionally like MNT{4,6}-753 Make CI conditional on which curves are changed
2 participants