exercism / ocaml

Exercism exercises in OCaml.
https://exercism.org/tracks/ocaml
MIT License
94 stars 50 forks source link

Palindrome_products Compilation Issue #496

Closed aeliusrs closed 11 months ago

aeliusrs commented 11 months ago

Hey,

I'm currently using OCaml 5 on my linux laptop and running make was failing because ppx_deriving was not installed.

So I did installed it with opam install ppx_deriving, problem solved.

But on the online test when you submit your iteration it will failed because of the same issue.

I believe this is a new bug, and it could be easily fix.

Either by removing the preprocess instruction in dune either by install ppx_deriving in the test environment

Thank y'all

georgyo commented 11 months ago

Good catch, looks like ppx_deriving was removed in this commit for some reason: https://github.com/exercism/ocaml-test-runner/pull/50

georgyo commented 11 months ago

I am not sure why we ever had the user implementing these helper functions for tests which are fairly unrelated to the actual exercise.

PRs created to add back in the PPXs and make it so users don't need to implement these functions at all.

aeliusrs commented 11 months ago

thank you !

i need to read the manual about how to interact with exercism track to contribute ;)

georgyo commented 11 months ago

These features have been merged, you should be good to go!

aeliusrs commented 11 months ago

Hey @georgyo, the test is now running correctly, but it timeout, if i remember this is not the first OCaml exercise to time out there is also "List Ops", any idea how to fix this ?

May I help ?

georgyo commented 11 months ago

Interesting, the problem here is that the test runner is given a total of 20seconds to run and running these tests is taking more than 20 seconds.

I don't think we can increase the timeout, so our only options is to make the tests run faster for this problem.

If you think you have ideas on how to make it faster feel free to send a PR.

aeliusrs commented 11 months ago

What is the CPU / RAM of the runner ?

because on my machine without any optimization I've got this :

time dune runtest
.............
Ran: 13 tests in: 0.11 seconds.
OK

real    0m9.324s
user    0m9.141s
sys     0m0.153s

I'm able to optimize the time by changing the compilation flag and using the optimized compiler in the dune file, such as:

(executable
 (name test)
 (libraries base ounit2))

(alias
 (name runtest)
 (deps
  (:< test.exe))
 (action
  (run %{<})))

(alias
  (name    buildtest)
  (deps    (:x test.exe)))

(env
  (dev
    (flags (:standard -warn-error -A -ccopt -O3))))

the result is now :

time dune runtest
.............
Ran: 13 tests in: 0.11 seconds.
OK

real    0m9.048s
user    0m8.881s
sys     0m0.133s

and of course you can even be more efficient by adding the jobs flag for dune run test: dune runtest -j 4

The two problematic test are the following:

  "find the smallest palindrome from four digit factors" >::
  ae (Ok {value=(Some 1002001); factors=[(1001,1001)]})
    (smallest ~min:1000 ~max:9999);
  "find the largest palindrome from four digit factors" >::
  ae (Ok {value=(Some 99000099); factors=[(9901,9999)]})
    (largest ~min:1000 ~max:9999);

Just by decreasing the min value of the second test we can cut the test time by two

  "find the largest palindrome from four digit factors" >::
  ae (Ok {value=(Some 99000099); factors=[(9901,9999)]})
    (largest ~min:8000 ~max:9999);
time dune runtest
.............
Ran: 13 tests in: 0.11 seconds.
OK

real    0m5.083s
user    0m4.902s
sys     0m0.134s

and actually i edit the test such as

  "find the smallest palindrome from four digit factors" >::
  ae (Ok {value=(Some 1002001); factors=[(1001,1001)]})
    (smallest ~min:1000 ~max:5000);
  "find the largest palindrome from four digit factors" >::
  ae (Ok {value=(Some 99000099); factors=[(9901,9999)]})
    (largest ~min:5000 ~max:9999);

and now the time without compilation flag of using jobs flag is :

time dune runtest
.............
Ran: 13 tests in: 0.11 seconds.
OK

real    0m2.831s
user    0m2.650s
sys     0m0.124s

Do you think this is valuable to do a PR ?

georgyo commented 11 months ago

I think it would be a valuable PR, but these tests come from a template, so you'll need to do some more wizardary than just editing the test.

https://github.com/exercism/ocaml/blob/main/templates/palindrome-products/test.ml.tpl

This template is populated from the canonical data for all exercism problem here: https://github.com/exercism/problem-specifications/blob/89237c5ddafb5e0c4954d7cc4805a91d165a95a1/exercises/palindrome-products/canonical-data.json

with special cases defined here: https://github.com/exercism/ocaml/blob/main/test-generator/lib_generator/special_cases.ml#L182

That is to say that you can likely edit the special cases for the generator and to edit the tests.

aeliusrs commented 11 months ago

Thank you for the explaination, let me check what i can do :)

georgyo commented 11 months ago

I'll add that just disabling/excluding those tests is likely a fine way to go about fixing this.

aeliusrs commented 11 months ago

Oh Ok thank !

I did not have time to do a PR, the template format for the test and exlusions is not as straight forward to understand, I did not have time to understand it fully ahaha

Let me know when you do the PR

There is also one test messing up in List Ops exercise, I can indicate more precisely if you need, let me know !