coq / coq-bench

Scripts for differential performance testing of Coq packages / versions
Other
4 stars 6 forks source link

Don't require package list to be topologically sorted #76

Closed JasonGross closed 4 years ago

JasonGross commented 4 years ago

Instead we perform a stable sort using the dependencies generated by paring the output of opam install --show-actions. Note that we depend on the output actions matching ∗\s*install\s*<package-name-without-spaces>. Note the special unicode star character. Perhaps we should only look for ^\s*.\s*intstall\s* instead, in case opam changes what character they use to indicate an install action? Or \sinstall\s*, in case the character goes away entirely? (But then we have a bit of trouble if we ever try to install a package called "install".)

We do a manual sort because we want to preserve user-ordering insofar as it is possible. In particular, we want to allow bench initiators to declare that certain packages should be installed as late as possible, by putting them at the end of the list, so that if they interrupt the bench early, they get the packages they care most about. (I could not figure out how to get opam install --show-actions to be a stable sort.)

Closes #3

ejgallego commented 4 years ago

Why not to use the OCaml API for OPAM to compute this in a more principled [and hack-free] way?

JasonGross commented 4 years ago

Because I was not aware it existed. A quick Google search does not give me a reference for it; could you point me at it?

ejgallego commented 4 years ago

Yeah the API is not very well advertised even if in quite wide use; docs are here:

https://opam.ocaml.org/doc/api/

ejgallego commented 4 years ago

This is the opam file: https://opam.ocaml.org/doc/api/opam-format/OpamFile/OPAM/index.html

ejgallego commented 4 years ago

CI seems broken, let me try to fix it.

JasonGross commented 4 years ago

@ejgallego Interfacing with the API is non-obvious to me, and I'm not currently inclined to figure out how to make use of it. The current thing here works, and while it'd be ideal to use the API, I think it's probably worth merging as-is unless someone else wants to write a version that uses the API. Here's an incomplete list of things I'm not sure how to do:

ejgallego commented 4 years ago

@JasonGross in order to interact the with the API you should set a proper OCaml project using dune, that's fairly easy to do, let me know if you need help. That will also fix the tuareg issues.

For the last point, I could have a look, but it doesn't look too challenging to me.

Regarding the PR itself, I feel a bit conflicted: the amount of technical debt it introduces seems too much for the benefits of this feature; keep in mind that the bash-based script is scheduled to be deprecated in favor of a pure OCaml solution. I am unable to maintained the shell-based code.

That being said I'm OK merging as-is if you want, the worst that could happen is that we drop this code in the future.

JasonGross commented 4 years ago

@ejgallego If you'd be willing to take a look at getting the OCaml-side-opam to work (or as much of it as you're willing to), I'd be quite appreciative. (For example, I don't see any function that returns OpamStateTypes.switch_state, which is needed as an input for resolve.)

Note that the bash script is not doing much interesting. It's just returning, for each package $pkg with a comma-separated-list of install-actions $pkg_actions, the string $pkg:$pkg_actions.

Regarding making this be all ocaml-based, if you have

(* Returns a list of packages to be installed when installing the package given as the argument *)
val get_opam_install_actions : string -> string list

then you can adapt my ocaml script to work by replacing

-let get_pkg_name arg =
-  List.nth (String.split_on_char ':' arg) 0
-
-let get_pkg_deps arg =
-  String.split_on_char ',' (List.nth (String.split_on_char ':' arg) 1)
-
-let split_pkg arg = get_pkg_name arg, get_pkg_deps arg
-
-let depends_on arg1 arg2 =
+let depends_on (pkg1, deps1) (pkg2, deps2) =
-  let pkg1, deps1 = split_pkg arg1 in
-  let pkg2, deps2 = split_pkg arg2 in
  pkg1 != pkg2 && List.mem pkg2 deps1

and also by doing

let main () =
  let args = Array.to_list Sys.argv in
-  let pkgs = List.tl args in
+  let pkgs = List.map (fun pkg -> (pkg, get_opam_install_actions pkg)) (List.tl args) in
  let sorted_pkgs = sort (pkgs, []) in
-  Printf.printf "%s\n%!" (String.concat " " (List.map get_pkg_name sorted_pkgs))
+  Printf.printf "%s\n%!" (String.concat " " (List.map (fun (pkg, _) -> pkg) sorted_pkgs))
ejgallego commented 4 years ago

Thanks @JasonGross , indeed I'll have a look, but not before 2020 I think.

So if you want this in, I'll say let's merge then, an open a ticket for the OCaml migration.

JasonGross commented 4 years ago

So if you want this in, I'll say let's merge then, an open a ticket for the OCaml migration.

That sounds good, thanks!

ejgallego commented 4 years ago

We should update the documentation, I'm unsure where the statement is tho , https://github.com/coq/coq/wiki/Jenkins-(automated-benchmarking) ?

JasonGross commented 4 years ago

The statement is in the description of coq_opam_packages at https://ci.inria.fr/coq/view/opam/job/benchmark-part-of-the-branch/configure: image I'll update it now

ejgallego commented 4 years ago

Thanks @JasonGross