[Compiler-v2] run `move_pr.sh -2` in CI #15200

rahxephon89 · 2024-11-05T23:35:54Z

Description

This PR adds execution of the script move_pr.sh -2 to CI.

close #13750

How Has This Been Tested?

Key Areas to Review

need to check updated docs and exp files;
whether the script execution is flaky.

Type of Change

Which Components or Systems Does This Change Impact?

Checklist

I have read and followed the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I identified and added all stakeholders and component owners affected by this change as reviewers
I tested both happy and unhappy path of the functionality
I have made corresponding changes to the documentation

trunk-io · 2024-11-05T23:35:57Z

⏱️ 2h 51m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
rust-move-tests	31m	🟩
rust-move-tests	29m	🟩
rust-move-tests	24m	🟥
rust-move-tests	23m	🟩
forge-framework-upgrade-test / forge	22m	🟥 🟥
rust-move-tests	18m	🟥
rust-cargo-deny	9m	🟩 🟩 🟩 🟩 🟩
check-dynamic-deps	5m	🟩 🟩 🟩 🟩 🟩
check	4m	🟩
general-lints	2m	🟩 🟩 🟩 🟩 🟩
semgrep/ci	2m	🟩 🟩 🟩 🟩 🟩
file_change_determinator	51s	🟩 🟩 🟩 🟩 🟩
permission-check	14s	🟩 🟩 🟩 🟩 🟩
permission-check	11s	🟩 🟩 🟩 🟩 🟩

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

brmataptos

I have some slight concerns that using both -i and -2 in the same run leads to more limited exercise of framework compilation with MOVE_COMPILER_V2 enabled, since we don't do a cargo clean between the two runs. Just FYI.

brmataptos

Something is better than nothing.

rahxephon89 · 2024-11-13T23:02:43Z

I have some slight concerns that using both -i and -2 in the same run leads to more limited exercise of framework compilation with MOVE_COMPILER_V2 enabled, since we don't do a cargo clean between the two runs. Just FYI.

Any suggestion on how to set the flag, e.g, just using -2? @brmataptos

brmataptos · 2024-11-13T23:33:28Z

If I really want clean results, I use 2 runs, one with -i, one with -2. We might change the script, e.g., to run cargo clean -p aptos-framework, in between the two steps. Or maybe my fears are overblown and we can just leave it. Just something to look out for.

vineethk · 2024-11-13T23:48:09Z

.github/actions/move-tests-compiler-v2/action.yaml

-    - name: Run Aptos Move tests with compiler V2
-      run: cargo x targeted-compiler-v2-tests -vv --release --profile ci --locked --no-fail-fast
+    # Run move_pr.sh -i2
+    - name: Run move_pr.sh with V2 flag


Suggested change

- name: Run move_pr.sh with V2 flag

- name: Run move_pr.sh using both compiler v1 and v2 for integration tests

graphite-app · 2024-11-14T05:28:30Z

aptos-move/framework/aptos-framework/tests/aggregator_tests.move

@@ -12,7 +12,7 @@ module aptos_framework::aggregator_tests {
        let aggregator = aggregator_factory::create_aggregator_for_test();

        aggregator::add(&mut aggregator, 12);
-        assert!(aggregator::read(&aggregator) == 12, 0);
+        assert!(aggregator::read(&aggregator) != 12, 0);


This assertion appears to be testing the wrong condition. After adding 12 to the aggregator, reading its value should yield 12. The current assertion using != will fail the correct behavior and pass the incorrect one. The assertion should be:

assert!(aggregator::read(&aggregator) == 12, 0);

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2024-11-14T06:48:56Z

.github/actions/move-tests-compiler-v2/action.yaml

+    - name: Run move_pr.sh for integration tests with v2 compiler
      shell: bash
+      run: third_party/move/scripts/move_pr.sh -2


The PR description indicates that both move_pr.sh -i and move_pr.sh -2 should be run in CI, but this change only implements the -2 variant. If running both variants is still desired, consider adding a separate step for move_pr.sh -i. Otherwise, please update the PR description to match the intended implementation.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

rahxephon89 · 2024-11-14T06:50:12Z

@vineethk @brmataptos I removed the flag -i since it is for testing compiler v1.

brmataptos · 2024-11-14T06:59:50Z

Assuming that we're testing move-compiler-v2 in some other script, sounds fine.

github-actions · 2024-11-15T02:15:51Z

✅ Forge suite `realistic_env_max_load` success on `64a75a92c46ca531d1a01a9323063b77851e1943`

two traffics test: inner traffic : committed: 14258.29 txn/s, latency: 2789.85 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3300 ms), latency samples: 5421320
two traffics test : committed: 100.07 txn/s, latency: 1408.62 ms, (p50: 1400 ms, p70: 1400, p90: 1500 ms, p99: 1700 ms), latency samples: 1780
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 2.077, avg: 1.597", "ConsensusProposalToOrdered: max: 0.332, avg: 0.296", "ConsensusOrderedToCommit: max: 0.377, avg: 0.363", "ConsensusProposalToCommit: max: 0.671, avg: 0.659"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.03s no progress at version 2564096 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.50s no progress at version 2564094 (avg 8.50s) [limit 15].
Test Ok

github-actions · 2024-11-15T02:17:31Z

✅ Forge suite `framework_upgrade` success on `2bb2d43037a93d883729869d65c7c6c75b028fa1` ==> `64a75a92c46ca531d1a01a9323063b77851e1943`

Compatibility test results for 2bb2d43037a93d883729869d65c7c6c75b028fa1 ==> 64a75a92c46ca531d1a01a9323063b77851e1943 (PR)
Upgrade the nodes to version: 64a75a92c46ca531d1a01a9323063b77851e1943
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1280.94 txn/s, submitted: 1283.58 txn/s, failed submission: 2.63 txn/s, expired: 2.63 txn/s, latency: 2430.28 ms, (p50: 2100 ms, p70: 2400, p90: 3900 ms, p99: 5700 ms), latency samples: 116800
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1333.77 txn/s, submitted: 1336.22 txn/s, failed submission: 2.44 txn/s, expired: 2.44 txn/s, latency: 2274.37 ms, (p50: 2100 ms, p70: 2400, p90: 3600 ms, p99: 5400 ms), latency samples: 120080
5. check swarm health
Compatibility test for 2bb2d43037a93d883729869d65c7c6c75b028fa1 ==> 64a75a92c46ca531d1a01a9323063b77851e1943 passed
Upgrade the remaining nodes to version: 64a75a92c46ca531d1a01a9323063b77851e1943
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1386.44 txn/s, submitted: 1388.64 txn/s, failed submission: 2.19 txn/s, expired: 2.19 txn/s, latency: 2210.92 ms, (p50: 2100 ms, p70: 2400, p90: 3300 ms, p99: 4800 ms), latency samples: 126340
Test Ok

github-actions · 2024-11-15T02:17:34Z

✅ Forge suite `compat` success on `2bb2d43037a93d883729869d65c7c6c75b028fa1` ==> `64a75a92c46ca531d1a01a9323063b77851e1943`

Compatibility test results for 2bb2d43037a93d883729869d65c7c6c75b028fa1 ==> 64a75a92c46ca531d1a01a9323063b77851e1943 (PR)
1. Check liveness of validators at old version: 2bb2d43037a93d883729869d65c7c6c75b028fa1
compatibility::simple-validator-upgrade::liveness-check : committed: 17451.45 txn/s, latency: 1951.86 ms, (p50: 2000 ms, p70: 2100, p90: 2200 ms, p99: 2500 ms), latency samples: 561020
2. Upgrading first Validator to new version: 64a75a92c46ca531d1a01a9323063b77851e1943
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7342.29 txn/s, latency: 3875.41 ms, (p50: 4400 ms, p70: 4600, p90: 4700 ms, p99: 4800 ms), latency samples: 135880
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6755.80 txn/s, latency: 4358.89 ms, (p50: 4700 ms, p70: 4700, p90: 4800 ms, p99: 5000 ms), latency samples: 251920
3. Upgrading rest of first batch to new version: 64a75a92c46ca531d1a01a9323063b77851e1943
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7504.06 txn/s, latency: 3639.30 ms, (p50: 4100 ms, p70: 4500, p90: 5000 ms, p99: 5200 ms), latency samples: 134600
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7789.67 txn/s, latency: 4136.39 ms, (p50: 4300 ms, p70: 4400, p90: 6100 ms, p99: 6500 ms), latency samples: 257000
4. upgrading second batch to new version: 64a75a92c46ca531d1a01a9323063b77851e1943
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 12127.52 txn/s, latency: 2287.67 ms, (p50: 2500 ms, p70: 2600, p90: 2700 ms, p99: 2900 ms), latency samples: 214400
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 10508.91 txn/s, latency: 2998.25 ms, (p50: 2700 ms, p70: 2800, p90: 5700 ms, p99: 7400 ms), latency samples: 369640
5. check swarm health
Compatibility test for 2bb2d43037a93d883729869d65c7c6c75b028fa1 ==> 64a75a92c46ca531d1a01a9323063b77851e1943 passed
Test Ok

rahxephon89 force-pushed the teng/update-v2-move-pr branch 2 times, most recently from 47ae154 to 60d6d51 Compare November 6, 2024 04:20

rahxephon89 changed the title ~~[WIP][Compiler-v2] update by running move_pr.sh -i2~~ [Compiler-v2] update by running move_pr.sh -i2 Nov 6, 2024

rahxephon89 changed the title ~~[Compiler-v2] update by running move_pr.sh -i2~~ [Compiler-v2] run move_pr.sh -i2 in CI Nov 6, 2024

rahxephon89 marked this pull request as ready for review November 6, 2024 05:25

rahxephon89 requested review from areshand, davidiw, movekevin, wrwg and a team as code owners November 6, 2024 05:25

rahxephon89 force-pushed the teng/update-v2-move-pr branch 2 times, most recently from 2179d26 to 2da6849 Compare November 13, 2024 20:39

rahxephon89 requested review from vineethk and brmataptos November 13, 2024 20:40

brmataptos reviewed Nov 13, 2024

View reviewed changes

brmataptos approved these changes Nov 13, 2024

View reviewed changes

vineethk approved these changes Nov 13, 2024

View reviewed changes

rahxephon89 added 2 commits November 13, 2024 17:21

update v2

0a5c2a7

separate -i and -2

5f90b18

rahxephon89 force-pushed the teng/update-v2-move-pr branch from 2da6849 to 5f90b18 Compare November 14, 2024 01:21

rahxephon89 changed the title ~~[Compiler-v2] run move_pr.sh -i2 in CI~~ [Compiler-v2] run move_pr.sh in CI Nov 14, 2024

graphite-app bot reviewed Nov 14, 2024

View reviewed changes

remove -i

11abca6

rahxephon89 force-pushed the teng/update-v2-move-pr branch from c9ea32d to 11abca6 Compare November 14, 2024 06:48

rahxephon89 changed the title ~~[Compiler-v2] run move_pr.sh in CI~~ [Compiler-v2] run move_pr.sh -2 in CI Nov 14, 2024

graphite-app bot reviewed Nov 14, 2024

View reviewed changes

rahxephon89 enabled auto-merge (squash) November 14, 2024 23:08

This comment has been minimized.

Sign in to view

Merge branch 'main' into teng/update-v2-move-pr

64a75a9

This comment has been minimized.

Sign in to view

rahxephon89 merged commit d7e9d70 into main Nov 15, 2024
48 checks passed

rahxephon89 deleted the teng/update-v2-move-pr branch November 15, 2024 02:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Compiler-v2] run `move_pr.sh -2` in CI #15200

[Compiler-v2] run `move_pr.sh -2` in CI #15200

rahxephon89 commented Nov 5, 2024 •

edited

Loading

trunk-io bot commented Nov 5, 2024 •

edited

Loading

brmataptos left a comment

brmataptos left a comment

rahxephon89 commented Nov 13, 2024

brmataptos commented Nov 13, 2024

vineethk Nov 13, 2024

graphite-app bot Nov 14, 2024

graphite-app bot Nov 14, 2024

rahxephon89 commented Nov 14, 2024

brmataptos commented Nov 14, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Nov 15, 2024

github-actions bot commented Nov 15, 2024

github-actions bot commented Nov 15, 2024

	- name: Run move_pr.sh with V2 flag
	- name: Run move_pr.sh using both compiler v1 and v2 for integration tests

[Compiler-v2] run move_pr.sh -2 in CI #15200

[Compiler-v2] run move_pr.sh -2 in CI #15200

Conversation

rahxephon89 commented Nov 5, 2024 • edited Loading

Description

How Has This Been Tested?

Key Areas to Review

Type of Change

Which Components or Systems Does This Change Impact?

Checklist

trunk-io bot commented Nov 5, 2024 • edited Loading

brmataptos left a comment

Choose a reason for hiding this comment

brmataptos left a comment

Choose a reason for hiding this comment

rahxephon89 commented Nov 13, 2024

brmataptos commented Nov 13, 2024

vineethk Nov 13, 2024

Choose a reason for hiding this comment

graphite-app bot Nov 14, 2024

Choose a reason for hiding this comment

graphite-app bot Nov 14, 2024

Choose a reason for hiding this comment

rahxephon89 commented Nov 14, 2024

brmataptos commented Nov 14, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Nov 15, 2024

✅ Forge suite realistic_env_max_load success on 64a75a92c46ca531d1a01a9323063b77851e1943

github-actions bot commented Nov 15, 2024

✅ Forge suite framework_upgrade success on 2bb2d43037a93d883729869d65c7c6c75b028fa1 ==> 64a75a92c46ca531d1a01a9323063b77851e1943

github-actions bot commented Nov 15, 2024

✅ Forge suite compat success on 2bb2d43037a93d883729869d65c7c6c75b028fa1 ==> 64a75a92c46ca531d1a01a9323063b77851e1943

[Compiler-v2] run `move_pr.sh -2` in CI #15200

[Compiler-v2] run `move_pr.sh -2` in CI #15200

rahxephon89 commented Nov 5, 2024 •

edited

Loading

trunk-io bot commented Nov 5, 2024 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `64a75a92c46ca531d1a01a9323063b77851e1943`

✅ Forge suite `framework_upgrade` success on `2bb2d43037a93d883729869d65c7c6c75b028fa1` ==> `64a75a92c46ca531d1a01a9323063b77851e1943`

✅ Forge suite `compat` success on `2bb2d43037a93d883729869d65c7c6c75b028fa1` ==> `64a75a92c46ca531d1a01a9323063b77851e1943`