Skip to content

The ok_ko test takes an unreasonable amount of RAM #1229

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
kit-ty-kate opened this issue Apr 1, 2025 · 14 comments · Fixed by ocaml/opam-repository#27728
Closed

The ok_ko test takes an unreasonable amount of RAM #1229

kit-ty-kate opened this issue Apr 1, 2025 · 14 comments · Fixed by ocaml/opam-repository#27728

Comments

@kit-ty-kate
Copy link

The test located in tests/ok_ko.ml takes >8GB alone on my machine.
If this amount of RAM is necessary, maybe it would be worth splitting into its own test disabled by default so that the various CI systems running on opam-repository do not run into this problem.

To reproduce: run opam install -j1 -t lambdapi.2.6.0 and survey the RAM usage of the soon to spawn ok_ko.exe process.

lambdapi: 2.6.0
OCaml: 4.14.2
Platform: Linux/x86_64

@fblanqui
Copy link
Member

fblanqui commented Apr 3, 2025

Hi. How to know how much RAM is used by a process?

@kit-ty-kate
Copy link
Author

ps -ewwo 'rss=,cmd=' | grep ok_ko.exe will show you how much ram (in KB) is used by the process at a given time.

@fblanqui
Copy link
Member

fblanqui commented Apr 3, 2025

Hi. I cannot reproduce your problem. On my machine, dune runtest takes 13s and about 2 Gb only. What are the characteristics of your machine?

@kit-ty-kate
Copy link
Author

2GB sounds already pretty high for a single test case registered in opam-repository in my opinion.
What's your sample rate for the ps command though? There might be a short peak memory usage that gets freed fast somehow.

I'm running the command above in a docker container (ocaml/opam:debian-12) via opam-health-check-ng which doesn't have any fancy docker run arguments aside from --memory=10000m --network=none

@fblanqui
Copy link
Member

fblanqui commented Apr 4, 2025

Anyway, you are right: we do not need to run the tests on opam anyway. It's useful for CI only. We are going to remove them.

@fblanqui
Copy link
Member

fblanqui commented Apr 4, 2025

To this end, is it enough to remove the line " "@runtest" {with-test}" in lambdapi.opam? Should I update all the opam files in https://github.com/ocaml/opam as well?

@ejgallego
Copy link
Collaborator

@fblanqui , I think it is good that you keep some tests in the opam part of the package, under the runtest alias.

This means you can keep the standard opam layout and you can still benefit from the very nice opam CI infrastructure.

What I'd suggest is you define a new alias: @tests-ci or @tests-large, then you can run these tests with dune build @test-large in your CI. (Recall that @runtest is just a regular alias)

@fblanqui
Copy link
Member

fblanqui commented Apr 8, 2025

@ejgallego Thanks for your remark. Tests are run in the Lambdapi CI. So there is no need to add them on Opam again. Actually, in previous versions of Lambdapi, opam files did not include any tests. I don't remember why this has been included in the last release.

@kit-ty-kate
Copy link
Author

Tests in opam are useful though. It allows to the opam-repo-ci to see breakages in dependencies of lambdapi more consistently

@shonfeder
Copy link

Hello! I'm chiming in here as part of reviewing ocaml/opam-repository#27728 -- I'd like to echo @kit-ty-kate with a bit more detail on the advantages of running some core tests on the opam repository:

  • We test against a quite large matrix of platforms, and often catch OS- or architecture-specific breakage that goes undetected in development CI pipelines.
  • We test reverse dependencies, so whenever there is an proposed to one of Lambdapi's dependencies, the opam CI build and tests this package. If breakage unintended breakage is detected, developers will fix the package before publishing, and if it is an intended breaking change, we will proactively add upper bound limits to ensure lambdapi continues to build and install correctly. While we can of course detect type-level breakage just by compiling, we need your tests to be able to detect possible breakage to behavior that is not fully specified in the types or which only affects your test dependencies.

This benefits not only help Lambdapi in particular, but also have accumulative effects to ensure a stable and harmonious ecosystem of packages.

If you are willing to reconsider, I can lend a hand to help factor out the less expensive tests into something that can be run during installation.

@fblanqui
Copy link
Member

Hi. Thank you for your detailed explanations. We'll try to add back some small tests later.

@ejgallego
Copy link
Collaborator

Indeed thanks folks for the detailed explanations, I was wondering if there is already documentation explaining this to package authors?

If that's not the case, I'd be happy to try to add a synthesis of the discussion for future reference, similar questions come often for example in Rocq.

@shonfeder
Copy link

shonfeder commented Apr 19, 2025

@ejgallego great question 😅 . I've created ocaml/opam-repository#27796 to discuss the point! Help on this would indeed be most welcome.

@fblanqui
Copy link
Member

Thanks for your remarks. I propose to close this issue as it is solved in ocaml/opam-repository#27728 and #1234 .

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 a pull request may close this issue.

4 participants