Implement pre-compilation of exercises and graders

[AltGr]

Close #478, Close #414, Close #205, Close #312, Close #158

[AltGr]

This replaces the prepare.ml, solution.ml and test.ml files that were sent scrambled and evaluated textually in the grader's toplevel by compiled files (interfaces in cmi, implementation in cma and js depending on the backend).

The main benefit is two-fold:

• it's now impossible to extract the solution (or grader) from a running server: only the compiled version — useless for cheating — is available

• it's much more efficient:

                            MASTER  PRECOMPILE
    build -j1               1005s   33s
    build -j12              (broken)7.6s
    loading exercise        5.8s    5.2s
    grading (100%)          6.4s    4.0s
  

(these have been run on the learn-ocaml-corpus repository and the markov exercise, respectively. Check my (temporary) patches here)

That's a 132x speedup on learn-ocaml build in the best case!

Todo:

• [x] re-implement depend.txt handling (or replace it with something more powerful, it fits much better in the new backend and will only be needed at build-time); and check that the (temporarily disabled) mutations_test.ml works with it
• [x] add the ppx to introduce the register_sampler calls that are now required (@hernoufM)
• [x] samplers are not properly type-checked yet
• [x] one grader of the corpus segfaults (week5/seq1/ex2) (but see point just above!)
• [x] asak/solution partitioning is disabled: if someone knowing that part of the code could have a look, @nobrakal ?
• [x] restore feature to define custom printers
• [ ] check how to handle the emacs-mode with the new server (@erikmd ?)
• [x] check that the small changes required in exercises are OK with users (in progress on the mailing-list)

[AltGr]

This should finally be all good feature-wise. Last bits:

• [x] Some documentation
• [x] Fix static builds / docker containers ; building an exercise repository now requires ocamlc to be available

[AltGr]

This is finalised and, I think, ready to review and go! Report on the status of the tests:

• :heavy_check_mark: building the static binaries works fine
• :heavy_check_mark: building the Docker images and running the dockerised tests work fine
• :orange_circle: Build learn-ocaml and run tests uses master of learn-ocaml-corpus. There are some small changes required, detailed in this PR. These should be backwards-compatible¹ and I can confirm running the test locally with that branch of the corpus works.
• :heavy_check_mark: Build learn-ocaml-client and run quick tests works on the current version image
• :red_circle: Build learn-ocaml-client and run quick tests (ocamlsf/learn-ocaml:0.12/0.13.0): these tests check for HTTP API compatibility with learn-ocaml 0.12 and 0.13.0, which is a non-goal of this PR (we don't want to provide solution.ml files in any form anymore!). A major version bump is in order, and this will only affect users of the command-line client.

¹ at the loss however of the installed printers, on older versions of learn-ocaml, since they relied on the #install_printer directive which we can't keep, and they don't have the new mechanism. If this is a problem we can have a separate branch in learn-ocaml corpus and point the test to that.

[AltGr]

~~Ah, I forgot to add the copyright headers to all the new files.~~ [EDIT: done]

[AltGr]

I know some teachers who were expecting this to be at least merged for their classes in June; can we go forward, or should I advise them to use an unofficial branch ?

[erikmd]

@AltGr thanks for your ping 👍. Sorry, was swamped with teaching lately, my plan actually is to integrate it as soon as possible once:

1. asak-partitioning implementation is updated (I will email Alexandre to arrange some meeting slot soonish)
2. learn-ocaml.el mode is updated (will be easy AFAICT, will let you know ASAP)
3. and ideally after discussing with you, to figure out some way to keep compatibility with learn-ocaml-editor! 🙏
4. and finally after a slight-multi-squash to be fully release-please compliant

As a result, I'm afraid the feature won't be released this month, but definitely in July for September.

[nobrakal]

Woops, I am really sorry for my lack of reply, I did not see the previous mention.

I wonder if anyone actually uses the partitioning feature, I hope this is not a release-blocker.

Anyway, even if I lack the time to implement any modification by myself, I am available to review or discuss any change if needed.

[erikmd]

Hi!

Woops, I am really sorry for my lack of reply, I did not see the previous mention.

No worries, @nobrakal !

I wonder if anyone actually uses the partitioning feature,

I think so (and hope so :)

I hope this is not a release-blocker.

But even if we don't have usage statistics, I think it is a "release-blocker", at least given the asak-partitioning feature was part of the previous stable releases of learn-ocaml!

Anyway, even if I lack the time to implement any modification by myself, I am available to review or discuss any change if needed.

OK, thanks… anyway we'll be able to discuss this with @yurug at the end of this week

[AltGr]

Having rushed to have this ready back in April, I have to say I am pretty disappointed that this hasn't been merged yet, esp. since I told a few teachers that it would be in a release for the semester start in September.

I understand the issue with Asak getting temporarily disabled, but, like @nobrakal, I don't think that should be a release blocker: this is a massive overall improvement for everyone (the 100x speedup on learn-ocaml build is no joke! And the solution hiding was requested by many for a long time) while Asak remains a niche feature. Don't get me wrong, it's a nice one, but if advertised correctly in the new release people who rely on it will know to stay on the current release until ported, while not holding the others back. The fact that the feature is not yet visible in the interface should help here, too (you have to know to middle-click on a specific item).

[AltGr]

@yurug I see you made attempt to repair Asak, thanks! did it work ? The patches make prepare.ml visible to learn-ocaml build but not further on a built instance and I was thinking that the partitioning was taking place on a running instance.

I don't remember precisely how Asak works but I think it should in theory be fine with just the cmi for prepare.ml ?

[erikmd]

Hi @AltGr, thanks a lot for your messages!, and for this very nice work :pray:

Having rushed to have this ready back in April, I have to say I am pretty disappointed that this hasn't been merged yet, esp. since I told a few teachers that it would be in a release for the semester start in September.

I fully understand—AFAICT, my OCaml lab sessions precisely start this week, and I would have loved to benefit from #481.

No need to tell many details about the "why", but Yann and I got much less time than expected lately.

BTW:

• If ever #475 had been implemented, PR #481 would immediately be "available" / easy to deploy by relying on static binaries, without any release. So I just labelled #475 as good first issue in passing 🙂

• Also the CI was broken till a few days ago… (the fix was simple; but before that moment, doing any release was impossible).

• Finally, you mentioned the asak issue (which is not an undocumented feature BTW 😉) but this is not the very last small point that remains to address.

• Anyway, @yurug and I restarted last week our recurrent learn-ocaml maintainer telco; so, no ETA; but stay tuned 👍

[gasche]

@erikmd and @yurug : gentle ping, have you been able to make progress on integrating this PR?

[erikmd]

gentle ping, have you been able to make progress on integrating this PR?

@gasche sure! as said previously, this PR is a priority! in particular as soon as the bugfix PR #506 is merged (certainly at our next learn-ocaml maintainer telco), we'll make a point release, then work on this PR #481. We'd also like to invite @AltGr at that point so that we can talk together with @yurug for the last steps of the review.

[yurug]

This PR seems to break the "partition view" feature. I suggest fixing this problem in a follow-up issue.

[erikmd]

• FYI I've made a backup of @AltGr's branch https://github.com/AltGr/learn-ocaml/tree/a7977b52

  → here: https://github.com/ocaml-sf/learn-ocaml/tree/backup-pr-481-AltGr (commit 64bcc95086634ac62289690c6077ed83d12b30a2)

• Then I've just rebased the branch on latest master to:

  • fix the conflicts (fortunately that was smooth)
  • squashed this commit into the first commit of the branch
  • reworded the (first line of) commit messages to follow the conventional-commits naming

• @yurug and I have now more time to focus on this priority PR, so stay tuned!

[AltGr]

Rebased

[AltGr]

3 CI jobs fail:

* The main test Build learn-ocaml and run tests successes on local tests then fails on the corpus

+ docker run --entrypoint '' -v /home/runner/work/learn-ocaml/learn-ocaml/tests/corpuses/learn-ocaml-corpus:/repository learn-ocaml /bin/sh -c 'learn-ocaml --repo=/repository build'
Learnocaml v.0.15.0 running.
Updating app at ./www
File "/repository/exercises/mooc/week1/seq4/ex2/prelude.ml", line 1:
Error: I/O error: /repository/exercises/mooc/week1/seq4/ex2/prelude.cmicd44e0.tmp: Permission denied
[etc.]

I couldn't reproduce locally but using docker -v is notably broken regarding file ownership so I guess there is some uid mismatch that makes the directory non-writeable. Probably nothing big to fix for a Docker guru ?

* Cross-version client tests for both 0.12 and 0.13 Build learn-ocaml-client and run quick tests (ocamlsf/learn-ocaml:0.13.0)

NOT OK: ./test_find_binding/find_binding/annot_all.ml
Reading solution...
Fetching exercise data from server...
Fatal error: exception Json_encoding.Cannot_destruct(_)

Incompatibilites at this level are to be expected (at least, older clients working on newer servers is a non-feature since they require an exposed solution file), so the compatibility matrix probably needs to be adjusted in the test ; but it's not totally obvious to me exactly what client/server combinations are failing here.

[nobrakal]

After a little discussion with @AltGr I was able to get asak working with this PR :)

• The relevant asak branch: https://github.com/nobrakal/asak/tree/parttype

  • The partition function takes now directly the type of the searched function (rather than a string representing the ML file containing the said solution): val create : int -> string -> Types.type_expr -> ('a * string) list -> 'a
  • I also added a function allowing to search the type of a particular function from a file's signature: val find_value_type_from_signature : string -> Types.signature_item list -> Types.type_expr

• I was able to incorporate asak's new interface in the PR. The code now extracts the signature of the solution from the relevant cmi, and calls the above functions: https://github.com/nobrakal/learn-ocaml/commit/249c6fe5163f79f426f73e7da2e83ee0be8bea66

Tell me if it sounds reasonable. If yes, I will release a new version of asak containing these modifications.

[AltGr]

This sounds very reasonable — a perfect solution actually. I'd have given you a "go" already for an asak release so that this can be merged, but @erikmd promised he would first check that his use-cases were working as expected.

[AltGr]

And thanks :)

[nobrakal]

Cool :D

Notice a small code duplication for the read_cmi_from_file function, where I adapted the code of load_cmi_from_string from toploop/toploop_ext.ml. I'm not sure where to put the common denominator.

[AltGr]

This sounds very reasonable — a perfect solution actually. I'd have given you a "go" already for an asak release so that this can be merged, but @erikmd promised he would first check that his use-cases were working as expected.

@nobrakal since it has been one month already, I'd say please go ahead with the release and we'll follow. Thanks again.

[AltGr]

@erikmd, this finalised version includes @nobrakal's patches so you should be able to check the partitioning feature directly with the static binary artefacts. If that works I don't think there could be any reason to delay merging this any longer.

[erikmd]

Hi @AltGr, thanks a lot for this impressive work!

If that works I don't think there could be any reason to delay merging this any longer.

Yes I see what you mean, thanks. We should just ensure that:

• we follow the roadmap we agreed on (in particular, releasing a point release before 1.0.0)
• learn-ocaml-mode's compatibility is updated (much easier than partition-view fortunately!)

regarding the point release I mention, the only missing patch is a full fix for #558. I already started working on it, we'll be able to discuss it at the telco.

[yurug]

I can assess that building an old repository of exercises of mine seems to work as expected.

[nobrakal]

@nobrakal since it has been one month already, I'd say please go ahead with the release and we'll follow. Thanks again.

After Asak 0.4 should lands opam's repository soon! https://github.com/ocaml/opam-repository/pull/24683

[AltGr]

Adjusted for now released asak.0.4 (removed the pin-depends in opam files)

[AltGr]

Ahem, CI failure is due to the latest corpus update, not this PR (it works outside of this CI, the exercise failing is the one making use of gg/vg so I guess it is a matter of installed libs in the container)

EDIT: fixed

[erikmd]

Hi @AltGr, thanks for the merge!, but please wait for the learn-ocaml.el and learn-ocaml-client fixes before we can release 1.0.0 :rocket:

[gasche]

Congratulations for the merge!