search

exercism / sml

Exercism exercises in Standard ML.
https://exercism.org/tracks/sml
MIT License
26 stars 33 forks source link

Speed up ci #223

Closed rainij closed 1 year ago

rainij commented 1 year ago

We improve the Run tests for all exercises step in the ci. Design goals are as follows

  1. Speed up the ci action (previously ~4 minutes, now ~20s), by not building polyml every time (resolves https://github.com/exercism/sml/issues/207).
  2. Do a proper integration test (compare: https://github.com/exercism/sml/issues/207#issuecomment-1158557150).

Goal 2 is achieved by using run.sh from within sml-test-runner image to do the actual test. This is the same script which is used when a student submits their exercise. To still satisfy Goal 1 we have to take care that docker is called only once, and not for every exercise separately since otherwise the overhead by spinning up the container (repeatedly) would dominate the time to execute the tests.

rainij commented 1 year ago

@ErikSchierboom maybe you could have a look into this MR?

I am mostly satisfied with the setup. But one thing keeps me thinking: Every ci run pulls the sml-test-runner image from dockerhub. The two (theoretical?) issues with that are:

  1. Takes a few extra seconds (with the new setup approximately half the time the ci needs).
  2. Dockerhub has a pull rate limit.

The first one is not such a big deal but the second one worries me a little bit. The sml track will probably never hit the limit alone (I assume 200 pulls in 6h). But probably sml shares the limit with other repos / all exercism?

I am not aware of anything which caches the pulled images. So the only approach which might solve the issue would be to checkout the sml-test-runner repo (or at least the Dockerfile and relevant build context resources) and building the image in this ci.yml as suggested here by you.

The abap repo, which you refer to in https://github.com/exercism/sml/issues/207#issuecomment-1164076823, does not do this and just pulls the image from dockerhub. So I wonder ...

What do you think about this? If it does not sound silly I would be glad to look into it / try it out here.

rainij commented 1 year ago

@ErikSchierboom thanks for the approval. I will merge soon, I have just one question you probably missed from my longer comment:

Could the line run: docker pull exercism/sml-test-runner in ci.yml lead to the pull-rate limit problem? They seem to have an "open source program" - not sure if exercism is part of that.

Even if this wasn't a problem right now I will probably keep this in mind since I feel it might be a nice thing to reduce resource usage (in this case load on dockerhub). Probably with some more knowledge about github actions one can avoid duplicating code when implementing the buildx approach for this repo (to avoid pulls) and for sml-test-runner. So far I don't know much about github actions.

ErikSchierboom commented 1 year ago

@rainij It could indeed lead to rate limits, but seeing how CI runs relatively infrequently for most tracks, I think we're fine. Once we start hitting rate limits, we can look at it again.

rainij commented 1 year ago

@rainij It could indeed lead to rate limits, but seeing how CI runs relatively infrequently for most tracks, I think we're fine. Once we start hitting rate limits, we can look at it again.

OK, then I leave it as is for now. But I keep it in mind - good to be prepared.