search

stan-dev / stanc3

The Stan transpiler (from Stan to C++ and beyond).
BSD 3-Clause "New" or "Revised" License
138 stars 44 forks source link

Update to OCaml 4.14 #1366

Closed WardBrian closed 8 months ago

WardBrian commented 9 months ago

This updates to OCaml 4.14, the last update of the 4.x release line. It is source-compatible with OCaml 5.0 if someone wishes to compile with that instead.

It also updates the Jane Street libraries to v0.16, which included migrating from Core_kernel to Core.

Finally, it also bumped some other dependencies:

This final change required some re-formatting, but with proper configuration it was not too bad.

Existing developers should install and use a new opam switch after this is merged: cd scripts/; ./setup_dev_env.sh

The required updates for building on Windows were done in: https://github.com/ocaml-cross/opam-cross-windows/pull/277 https://github.com/ocaml-cross/opam-cross-windows/pull/278 https://github.com/ocaml-cross/opam-cross-windows/pull/279 https://github.com/ocaml-cross/opam-cross-windows/pull/280 https://github.com/ocaml-cross/opam-cross-windows/pull/286

@serban-nicusor-toptal: this will need new versions of all of our docker images, and a new opam environment on the Jenkins mac. Let me know if you need any help!

Submission Checklist

Release notes

Updated the compiler to use OCaml 4.14 and newer versions of its dependencies.

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

serban-nicusor-toptal commented 9 months ago

Hey Brian, find the docker images at:

For the multiarch image, one of the deps seems to not be compatible with arm32 & 32bit

 > [linux/arm/v7 12/13] RUN curl https://raw.githubusercontent.com/stan-dev/stanc3/update/4.14/scripts/install_build_deps.sh | bash:
100   217  100   217    0     0    837      0 --:--:-- --:--:-- --:--:--   951
#75 1.167 [WARNING] Running as root is not recommended
#75 1.431 [WARNING] Running as root is not recommended
#75 3.667 core is now pinned to version v0.16.1
#75 3.800 [WARNING] Running as root is not recommended
#75 11.46 [ERROR] core = v0.16.1 unmet availability conditions: arch != "arm32" & arch != "x86_32"

Our target platforms for multi arch build are: --platform linux/arm/v6,linux/arm/v7,linux/arm64,linux/ppc64le,linux/mips64le,linux/s390x You can find the ci-scripts changes here: https://github.com/stan-dev/ci-scripts/pull/29

Is there a way we can fix that or do we ditch arm support ?

WardBrian commented 9 months ago

Can you try core 0.16.0 instead of 0.16.1? It doesn’t look like it has the same restriction in the metadata, but that doesn’t mean it will build necessarily.

WardBrian commented 9 months ago

See: https://github.com/ocaml/opam-repository/pull/23918

0.16.1 had this restriction added back, but it should be fine if those platforms use a different minor version

serban-nicusor-toptal commented 9 months ago

Nice! I'll rebuild, it might take a while ...

serban-nicusor-toptal commented 9 months ago

Looks like I got timeout for downloading so many packages haha, I'll try again in the morning. Do we update core v0.16.0 in the stanc3 PR too or leave this image with core v0.16.0 and the others core v0.16.1 ?

WardBrian commented 9 months ago

I think the main Linux image and the developers should still be 16.1

serban-nicusor-toptal commented 9 months ago

Hmmh, looks like it might not be in the repositories v0.16.0 ? I'm running into:

 > [linux/mips64le 13/19] RUN eval $(opam env) && opam install -y core.v0.16.0:
#81 0.469 [WARNING] Running as root is not recommended
#81 0.564 [WARNING] Running as root is not recommended
#81 194.8 Sorry, no solution found: there seems to be a problem with your request.
#81 194.8
#81 194.8 No solution found, exiting
------

Initially I thought it's a timeout but looks like it it's a missing package in the repo I think. Edit: I found it in the packages here https://opam.ocaml.org/packages/core/core.v0.16.0/ Edit: I think it might have something to do with the platform mips64le ? I've tried only ARM build and that worked fine.

WardBrian commented 9 months ago

Two things: was there an opam update before that, and is that colon at the end part of the command or just part of the display? (Hopefully the latter)

serban-nicusor-toptal commented 9 months ago

I've already tried with RUN eval $(opam env) && opam update && opam upgrade before the install package command. The colon is just a printing artifact, not in the command. If it matters, I managed to build all arch EXCEPT linux/mips64le which fails with the above. Edit: Pushed here stanorg/stanc3:multiarch-ocaml-4.14

WardBrian commented 9 months ago

While I'm debugging the mips64le build I decided to check, according to https://tooomm.github.io/github-release-stats/?username=stan-dev&repository=cmdstan, that architecture has only been downloaded 45 times total (across all versions) since we started offering it in 2.28

(for the record, the version we build for google collab has been downloaded 44 times for just 2.33.1, and the base cmdstan 2.33.1 ~15 thousand times)

If we need to disable builds on that platform for a little while I think that is OK

WardBrian commented 9 months ago

@andrjohns - you originally added the different arch builds for stanc. Was there a specific reason for the selection of archs? mips64el and s390x both seem relatively obscure

codecov[bot] commented 9 months ago

Codecov Report

Merging #1366 (6b62412) into master (4b8753a) will increase coverage by 0.48%. Report is 4 commits behind head on master. The diff coverage is 87.70%.

:exclamation: Current head 6b62412 differs from pull request most recent head 5e3f6c7. Consider uploading reports for the commit 5e3f6c7 to get more accurate results

@@            Coverage Diff             @@
##           master    #1366      +/-   ##
==========================================
+ Coverage   89.45%   89.93%   +0.48%     
==========================================
  Files          65       63       -2     
  Lines       10774    10704      -70     
==========================================
- Hits         9638     9627      -11     
+ Misses       1136     1077      -59
Files Coverage Δ
src/analysis_and_optimization/Dataflow_types.ml 0.00% <ø> (-8.70%) :arrow_down:
src/analysis_and_optimization/Dataflow_utils.ml 100.00% <100.00%> (ø)
...analysis_and_optimization/Debug_data_generation.ml 81.70% <100.00%> (-0.08%) :arrow_down:
...c/analysis_and_optimization/Dependence_analysis.ml 100.00% <100.00%> (ø)
src/common/FatalError.ml 0.00% <ø> (-66.67%) :arrow_down:
src/common/Fixed.ml 21.73% <ø> (-6.27%) :arrow_down:
src/common/Foldable.ml 11.76% <ø> (-9.29%) :arrow_down:
src/common/Gensym.ml 100.00% <100.00%> (ø)
src/common/Specialized.ml 75.00% <ø> (+8.33%) :arrow_up:
src/frontend/Ast.ml 68.69% <100.00%> (+11.61%) :arrow_up:
... and 50 more

... and 5 files with indirect coverage changes

andrjohns commented 9 months ago

@andrjohns - you originally added the different arch builds for stanc. Was there a specific reason for the selection of archs? mips64el and s390x both seem relatively obscure

Not particularly, it was just all archs supported by Debian. I figured that would cover the majority of use-cases

WardBrian commented 9 months ago

This is now building with mips64el disabled. @serban-nicusor-toptal is there a good way to test build the other binaries we normally only build on master?

serban-nicusor-toptal commented 9 months ago

One fast way would be to comment out the test stages, the whens for each binary build and run the pipeline. ( this can also be done in the Jenkins UI after hitting Replay on a pipeline without the need to commit here ). I can trigger it after this one finishes just so we test the binary builds.

WardBrian commented 9 months ago

Everything built fine, thanks @serban-nicusor-toptal for the tip

WardBrian commented 8 months ago

Thanks to some help from some Opam maintainers we determined that the issue with the mips64el architecture comes from the num library. The fix has already been merged into num, so it's just a matter of re-enabling after it gets released.

WardBrian commented 8 months ago

We actually just fixed mips64el! I just have to uncomment the Jenkins and test to make sure, but it should be the same as before now

WardBrian commented 8 months ago

Existing developers should install and use a new opam switch after this merge: git pull; cd scripts/; ./setup_dev_env.sh

Tagging those that come to mind: @SteveBronder @rok-cesnovar @andrjohns @spinkney