Skip to content

chore: [sc-126569] Upgrade to GHC 9.2 #7

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

Closed
wants to merge 7 commits into from

Conversation

codygman
Copy link
Contributor

@@ -54,7 +54,7 @@ RUN \

# Install GHC.

ARG GHC_VERSION=9.0.2
ARG GHC_VERSION=9.2.4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we not changing the default yet? I changed this because I assumed we were.

Copy link
Contributor

Choose a reason for hiding this comment

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

I hadn't changed the default yet because I wanted the default no-arg Docker build to work: #5 (comment)

@@ -54,7 +54,7 @@ RUN \

# Install GHC.

ARG GHC_VERSION=9.0.2
ARG GHC_VERSION=9.2.4
Copy link
Contributor

Choose a reason for hiding this comment

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

I hadn't changed the default yet because I wanted the default no-arg Docker build to work: #5 (comment)

Dockerfile Outdated
@@ -89,7 +89,7 @@ ARG HLS_VERSION=1.7.0.0
RUN \
set -o errexit -o xtrace; \
if test -n "$HLS_VERSION"; then \
ghcup install hls "$HLS_VERSION" --set; \
ghcup compile hls -g 7760340e --ghc $GHC_VERSION -j4 --cabal-update; \
Copy link
Contributor

Choose a reason for hiding this comment

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

I have some random thoughts about this:

  • Can we use long flags for everything? -g is --git-ref and -j is --jobs.
  • Similarly, can we use the complete Git commit hash of 7760340e999693d07fdbea49c9e20a3dd5458ad3?
  • Although I don't expect it to be necessary, can we be sure to quote the GHC_VERSION variable?
  • Should we use HLS_VERSION as the Git ref? That way 1.7.0.0 would work by targeting a tag and 7760340e would work by targeting a commit.
  • How long will this take? And does it make sense to set -j4 rather than -j? The build machine could have any number of cores available.

Dockerfile Outdated
@@ -89,7 +89,7 @@ ARG HLS_VERSION=1.7.0.0
RUN \
set -o errexit -o xtrace; \
if test -n "$HLS_VERSION"; then \
ghcup install hls "$HLS_VERSION" --set; \
ghcup compile hls -g 7760340e --ghc $GHC_VERSION -j4 --cabal-update; \
Copy link
Contributor

Choose a reason for hiding this comment

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

Aside from all those thoughts, my main concern here is bloating the Docker image. This is going to download the Hackage index and all of HLS's dependencies, then compile all of them. That's a lot of disk space. Would it be possible to clean up after this step?

Copy link
Contributor Author

@codygman codygman Aug 31, 2022

Choose a reason for hiding this comment

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

my main concern here is bloating the Docker image. This is going to download the Hackage index and all of HLS's dependencies, then compile all of them. That's a lot of disk space. Would it be possible to clean up after this step?

I'm pretty sure we did exactly that before to get rid of all the bloat we could from HLS, but in one of your refactors you removed it and changed to downloading the HLS binary.

I think you might have removed it because there were some libraries that couldn't be removed without breaking the compiled version of HLS and took up less but still substantial space.

Let me try to find that thread... I'm pretty sure I gave size numbers/etc.

Copy link
Contributor Author

@codygman codygman Aug 31, 2022

Choose a reason for hiding this comment

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

Okay I can't find that sadly.

I'm okay with doing all the work again, but first can we agree on a more objective metric than bloat?

The goal of keeping the docker-haskell image small is CI run times mainly right?

If so, can we go by a metric of "Adding compiled HLS must not slow down average 10 semaphore runs more than X seconds"?

Or perhaps I don't understand the metrics composed of your definition of bloat.

If that's the case I'd like to at least understand them better if not come up with an objective metric representing them before moving forward with this work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm currently compiling HLS for GHC 9.2.4 so that I can see how long it takes and how big the image is. So hold off on running your own tests until I'm done with that.

I don't have any objective metrics I'm shooting for. All else being equal, smaller images are better for a variety of reasons. They're faster to pull in CI, yes, but also they're faster to pull locally. That makes onboarding easier, and it makes upgrading easier. Also if you local environment gets trashed, it's easier to wipe everything and start again.

Clearly that needs to be balanced with utility though. For example in #1 I removed HLS for other versions of GHC, which was pure waste. But I also removed GHC's profiling libraries and documentation. Those could be useful! Are they worth their weight though? Based on how frequently we compile with profiling or browse local documentation, it seems like no, they're not worth their weight.

For compiling HLS from source, I suspect it's going to be possible to remove most (if not all) of the ephemera generated during the build. Hopefully that means the size of the image will be a moot point. But the time to build the image would remain a potential problem. It's not especially fast now at between 25 and 40 minutes in CI. But what if that, say, doubled? That might be alright, but then making any changes to this repo would be painful. Perhaps that's fine since we don't change this repo all that much.

Copy link
Contributor

Choose a reason for hiding this comment

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

# How long does it take to build this image on my machine?
$ time docker build --no-cache --build-arg GHC_VERSION=9.2.4 --build-arg HLS_VERSION= --tag ghcr.io/acilearning/docker-haskell:9.2.4-3d40f1c8d21bb63b7662b9b0b92d5a530692f762 .
...
Executed in   59.27 secs      fish           external
   usr time  239.80 millis    0.09 millis  239.71 millis
   sys time  181.97 millis    2.01 millis  179.96 millis

# How big is the resulting image?
$ docker image inspect ghcr.io/acilearning/docker-haskell:9.2.4-3d40f1c8d21bb63b7662b9b0b92d5a530692f762 | jq '.[0].Size'
2493485343
# 2,493,485,343 bytes = 2.3 GiB

# Set up a new image based on the old one that compiles HLS.
$ cat Dockerfile 
FROM ghcr.io/acilearning/docker-haskell:9.2.4-3d40f1c8d21bb63b7662b9b0b92d5a530692f762
RUN \
  set -o errexit -o xtrace; \
  ghcup compile hls \
    --cabal-update \
    --ghc 9.2.4 \
    --git-ref 7760340e999693d07fdbea49c9e20a3dd5458ad3; \
  haskell-language-server-wrapper --version

# How long does it take to build the new image?
$ time docker build --tag docker-haskell-pr-7 .
...
Executed in  551.91 secs    fish           external
   usr time    1.30 secs    0.09 millis    1.30 secs
   sys time    0.97 secs    1.82 millis    0.97 secs

# And how big is it?
$ docker image inspect docker-haskell-pr-7 | jq '.[0].Size'
5411816550
# 5,411,816,550 bytes = 5.0 GiB

Compiling the old image takes 59 seconds. Compiling the new one takes 552 seconds on top of that, for a total time of 611 seconds (or 10 minutes and 11 seconds).

The old image is 2.3 GiB and the new one is 5.0 GiB. So that's an added 2.7 GiB, which more than doubles the image size.

I didn't try at all to reduce the image size. I'm sure that there's an absolutely huge amount of stuff in ~/.cabal and /cabal-store that can be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried removing stuff with:

rm --recursive /cabal-store/* ~/.cabal/logs ~/.cabal/packages

That brought the size down to 2,716,333,619 bytes (2.5 GiB). That makes it 222,848,276 bytes (213 MiB) bigger than the base image.

$ ls -l ~/.ghcup/bin/haskell-language-server-wrapper-1.7.0.0 
-rwxr-xr-x 1 haskell haskell 222398880 Sep  1 19:56 /home/haskell/.ghcup/bin/haskell-language-server-wrapper-1.7.0.0

The HLS binary itself is 222,398,880 bytes (212 MiB). So that accounts for nearly all of the difference. The remaining 449,396 bytes (439 KiB) isn't worth worrying about to me.

@tfausak tfausak mentioned this pull request Sep 1, 2022
I couldn't get the inline variant to work after adding nested conditionals.
Weird this works fine for me locally
@tfausak
Copy link
Contributor

tfausak commented Sep 6, 2022

I think that the CI machine does not have enough resources (probably RAM) to compile HLS. It's possible you could trade RAM requirements for time by running with -j1, but I don't know how long it would take to complete. Builds are allowed to run for a long time.

@codygman
Copy link
Contributor Author

codygman commented Sep 6, 2022

I think that the CI machine does not have enough resources (probably RAM) to compile HLS. It's possible you could trade RAM requirements for time by running with -j1, but I don't know how long it would take to complete. Builds are allowed to run for a long time.

I made the -j1 change in b5c2aaa, can we look at enabling a job runner for github actions with like 16GB of memory?

https://docs.github.com/en/actions/using-github-hosted-runners/controlling-access-to-larger-runners

@codygman
Copy link
Contributor Author

codygman commented Sep 6, 2022

I was curious about how haskell-language-server compiles this, I think they use the haskell.org gitlab:

https://github.com/haskell/haskell-language-server/blob/master/.github/workflows/build.yml#L94

@tfausak
Copy link
Contributor

tfausak commented Sep 15, 2022

Closing since this was fixed by #9.

@tfausak tfausak closed this Sep 15, 2022
@tfausak tfausak deleted the sc-126569-hls-compiled-by-ghcup branch September 22, 2022 18:54
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.

2 participants