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

Only check with -Werror in CI #1431

Merged
merged 1 commit into from
May 15, 2024
Merged

Only check with -Werror in CI #1431

merged 1 commit into from
May 15, 2024

Conversation

locallycompact
Copy link
Contributor

@locallycompact locallycompact commented May 13, 2024

  • Adds a nix flake check CI job to run on each PR which now also builds all components with -Werror enabled.

  • Removes -Werror from cabal.project, to have the same behavior as nix build when building with cabal.


  • CHANGELOG updated or not needed
  • Documentation updated or not needed
  • Haddocks updated or not needed
  • No new TODOs introduced or explained herafter

@locallycompact locallycompact enabled auto-merge May 13, 2024 12:48
@locallycompact locallycompact force-pushed the lc/werror branch 8 times, most recently from 6f7a91c to b4f77ea Compare May 13, 2024 13:03
@locallycompact locallycompact requested a review from a team May 13, 2024 13:06
Copy link

github-actions bot commented May 13, 2024

Transactions Costs

Sizes and execution budgets for Hydra protocol transactions. Note that unlisted parameters are currently using arbitrary values and results are not fully deterministic and comparable to previous runs.

Metadata
Generated at 2024-05-14 20:14:49.60912344 UTC
Max. memory units 14000000
Max. CPU units 10000000000
Max. tx size (kB) 16384

Script summary

Name Hash Size (Bytes)
νInitial bccf2a430c016bc960fbf31b02694011cd399d20da8882aac9d33611 4110
νCommit 56b0f0b597150e619c76bed60683f3b1e42d7bc0685ed951b882bfc5 1975
νHead 86bff95ba20e9d1d1b34899a56d86bbacc9fed999260b27dcc92d128 9351
μHead 88f533cf67cd0fc93d7d9ccf0a8b1d69ffd1208a825efbebbc1d36ba* 4213
  • The minting policy hash is only usable for comparison. As the script is parameterized, the actual script is unique per Head.

Cost of Init Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 4792 8.89 3.40 0.46
2 4997 10.85 4.15 0.49
3 5194 12.83 4.92 0.52
5 5603 16.77 6.43 0.58
10 6605 26.16 10.02 0.72
48 14244 99.97 38.37 1.86

Cost of Commit Transaction

This is using ada-only outputs for better comparability.

UTxO Tx size % max Mem % max CPU Min fee ₳
1 559 10.24 4.04 0.29
2 746 13.88 5.64 0.34
3 931 17.66 7.29 0.39
5 1312 25.66 10.74 0.49
10 2247 48.19 20.30 0.78
19 3935 97.83 40.79 1.41

Cost of CollectCom Transaction

Parties UTxO (bytes) Tx size % max Mem % max CPU Min fee ₳
1 56 543 16.88 6.66 0.36
2 113 654 26.81 10.68 0.47
3 171 764 38.32 15.41 0.60
4 228 874 52.99 21.40 0.77
5 282 984 67.62 27.51 0.94
6 338 1095 82.76 33.88 1.11

Cost of Close Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 549 7.89 4.26 0.27
2 688 8.59 5.32 0.29
3 906 10.16 7.06 0.32
5 1233 12.06 9.56 0.37
10 1913 15.12 14.71 0.46
50 7915 48.98 62.15 1.34

Cost of Contest Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 627 8.65 4.73 0.28
2 758 9.30 5.77 0.30
3 842 9.62 6.51 0.31
5 1147 11.39 8.94 0.36
10 2017 16.70 15.66 0.49
50 8002 51.88 63.42 1.38

Cost of Abort Transaction

Some variation because of random mixture of still initial and already committed outputs.

Parties Tx size % max Mem % max CPU Min fee ₳
1 4669 17.00 7.35 0.55
2 4814 27.80 12.13 0.67
3 5052 43.43 19.16 0.86
4 5021 56.78 24.87 1.01
5 5088 70.57 30.80 1.17

Cost of FanOut Transaction

Involves spending head output and burning head tokens. Uses ada-only UTxO for better comparability.

Parties UTxO UTxO (bytes) Tx size % max Mem % max CPU Min fee ₳
5 0 0 4627 8.15 3.40 0.44
5 1 57 4661 9.63 4.26 0.46
5 5 284 4796 13.70 6.88 0.52
5 10 569 4967 19.16 10.31 0.60
5 20 1140 5307 30.69 17.44 0.76
5 30 1707 5647 42.22 24.58 0.92
5 40 2278 5987 54.17 31.89 1.08
5 50 2847 6326 65.71 39.04 1.24
5 80 4552 7341 99.94 60.31 1.72

End-To-End Benchmark Results

This page is intended to collect the latest end-to-end benchmarks results produced by Hydra's Continuous Integration system from the latest master code.

Please take those results with a grain of salt as they are currently produced from very limited cloud VMs and not controlled hardware. Instead of focusing on the absolute results, the emphasis should be on relative results, eg. how the timings for a scenario evolve as the code changes.

Generated at 2024-05-14 20:17:12.053817995 UTC

Baseline Scenario

Number of nodes 1
Number of txs 3000
Avg. Confirmation Time (ms) 4.372424411
P99 10.252372399999985ms
P95 5.649074799999998ms
P50 4.1060585ms
Number of Invalid txs 0

Three local nodes

Number of nodes 3
Number of txs 9000
Avg. Confirmation Time (ms) 21.798442137
P99 51.69125948000007ms
P95 30.327334949999983ms
P50 19.6583045ms
Number of Invalid txs 0

Copy link

github-actions bot commented May 13, 2024

Test Results

427 tests  ±0   419 ✅ ±0   15m 32s ⏱️ -31s
139 suites ±0     8 💤 ±0 
  2 files   ±0     0 ❌ ±0 

Results for commit 23eae35. ± Comparison against base commit cf535c7.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@ch1bo ch1bo left a comment

Choose a reason for hiding this comment

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

Why does this work? Why didn't it work before?

Our cabal.project does set -Werror on all local packages. So using cabal build I get the expected errors when doing:

rm -rf dist-newstyle && cabal build hydra-node

So why was the ci-nix.yaml workflow not failing on master? My hypothesis is that haskell.nix is not correctly picking up the project file and nix build .#hydra-node-tests does not fail (as does cabal build hydra-node above).

So my requested change: Clarify why it was not working before and fix it not only for nix flake check but for any builds through nix.

hydra-node/test/Hydra/Model/Payment.hs Show resolved Hide resolved
@locallycompact
Copy link
Contributor Author

It's categorically a bad user experience to fail a nix run command on a branch because of a warning.

flake.nix Outdated Show resolved Hide resolved
@ch1bo
Copy link
Collaborator

ch1bo commented May 14, 2024

It's categorically a bad user experience to fail a nix run command on a branch because of a warning.

I don't have such a strong opinion and would be fine with allowing warnings in all our nix build invocations, while in CI we do add the -Werror.

However, my request would then be to make the behavior consistent with how cabal build works. Most likely this means removing -Werror from cabal.project and updating our CONTRIBUTING.md to reflect that change. I suspect we should say something along the lines of "while warnings are not treated as errors during builds, CI will check for it before we merge any contributions"

@locallycompact locallycompact force-pushed the lc/werror branch 4 times, most recently from 67f2d5c to de067cf Compare May 14, 2024 12:17
@locallycompact locallycompact requested a review from ch1bo May 14, 2024 19:59
flake.nix Show resolved Hide resolved
@ch1bo ch1bo changed the title Add -Werror checks Only check with -Werror in CI May 15, 2024
@ch1bo
Copy link
Collaborator

ch1bo commented May 15, 2024

@locallycompact FYI I extended the PR description

@locallycompact locallycompact merged commit a08ab18 into master May 15, 2024
21 checks passed
@locallycompact locallycompact deleted the lc/werror branch May 15, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants