search

ocaml-bench / sandmark

A benchmark suite for the OCaml compiler
The Unlicense
82 stars 40 forks source link

Upgrade dune to 3.5.0 #421

Closed punchagan closed 1 year ago

punchagan commented 1 year ago

This PR removes the special version of dune installation for CI and use the same installation procedure as actual benchmark runs for CI as well, and upgrades the version of dune to 3.5.0

Without this PR, errors occur when trying to install hdr_histogram during the nightly runs (Expand the first diff to jump to the exact lines)

punchagan commented 1 year ago

Merging this would make sure we get rid of errors when running the parallel benchmarks, and copy the results to the main branch.

Firobe commented 1 year ago

Does this work with OCaml 4.14? In my last PR I tried to not change the overall version of dune bacause I remember @shakthimaan telling 4.14 was still tied to dune < 3.0

punchagan commented 1 year ago

Does this work with OCaml 4.14? In my last PR I tried to not change the overall version of dune bacause I remember @shakthimaan telling 4.14 was still tied to dune < 3.0

I'm not sure if it works with 4.14, I haven't tested it yet. But, from the opam dune page it looks like dune itself works with OCaml > 4.08.

Firobe commented 1 year ago

I tried on Navajo and 4.14 benchmarks seems broken even without this PR. @shakthimaan can we safely upgrade the dune version? Can we ditch OCaml 4.14? Where is 4.14 still used?

If yes, then other than my comment above the PR looks good to me!

punchagan commented 1 year ago

Yes, 4.14 is broken. Parallel benchmarks don't run because domainslib doesn't work with it. I have a WIP PR to get serial benchmarks running on 4.14 based on @shakthimaan 's work here: #417. Since it is just serial benchmarks, I suspect we shouldn't have much trouble with getting things working. In any case, I think it's valuable to have a build without errors on 5.1.0 and collecting the data from nightly runs to view in the UI. Any issues with 4.14 can be dealt with separately.

Firobe commented 1 year ago

I agree. If needed, another way to deal with the current nightly problem would be to not change anything and install dune.3.5.0 on the host switch on Turing instead (like done in CI currently), that's what causing the problem to begin with. However I don't see any file describing turing's switch and I can't access it.

punchagan commented 1 year ago

If needed, another way to deal with the current nightly problem would be to not change anything and install dune.3.5.0 on the host switch on Turing instead (like done in CI currently), that's what causing the problem to begin with. However I don't see any file describing turing's switch and I can't access it.

The problem is not Turing specific and Navajo has the same problem too. Upgrading to dune 3.5.0 feels like the right thing to do. Using a different version of dune on CI and when running the actual benchmarks is counter-productive and makes it harder for developers to work on Sandmark.

If we need to make tweaks for OCaml 4.14, we can, since it's already going to have some special stuff to not run parallel benchmarks, etc.

shakthimaan commented 1 year ago

Can we ditch OCaml 4.14?

No! We will need to continue to support 4.14 development branch because 5.0 is not released yet, and we need to give time for projects to migrate.

punchagan commented 1 year ago

No! We will need to continue to support 4.14 development branch because 5.0 is not released yet, and we need to give time for projects to migrate.

Sure. We can add back support for 4.14, but currently our nightly builds have errors in the logs and that causes the benchmark runs to be marked as errored runs and are not added on the main branch. I think it would be worth merging these changes and working on 4.14, separately. Thoughts, @shakthimaan ?

punchagan commented 1 year ago

We can hold off on merging this. I'm going to rebase the #417 PR to also add support for 4.14 and 5.0.0 with the new dune version, and that PR can be merged.

punchagan commented 1 year ago

Closed in favor of #417