`builder-db verify-cache-dir` #113

Merged
reynir merged 22 commits from 20220524_verify-cache-dir into main 2 months ago
rand commented 3 months ago
Owner
There is no content yet.
rand added 2 commits 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand changed title from WIP: `builder-db verify-cache-dir` to `builder-db verify-cache-dir` 3 months ago
rand added 1 commit 3 months ago
reynir reviewed 3 months ago
reynir left a comment

I tried to submit a review earlier, but it got rejected due to bad CSRF token(!!). I'm not confident none of my comment were lost...

I have a few comments. It seems to me that the code is maybe more complicated than it needs to be.

What I think could be useful is to write down in words what it is we want to check, and use that to guide the code.

The rresult library is sort of getting deprecated; most of the functionality exists in Result.

let dir_str = Fpath.to_string d in
let is_valid =
viz_types |> List.exists (fun viz_type ->
let prefix = Builder_web.Viz_aux.viz_type_to_string viz_type in
Poster
Owner

shouldn't it be prefix appended with _?

shouldn't it be `prefix` appended with `_`?
Poster
Owner

Ah, it is checked below. If we just ^ "_" here the code below can be a bit simpler.

Ah, it is checked below. If we just `^ "_"` here the code below can be a bit simpler.
rand marked this conversation as resolved
end in
match run_status with
| (cmd_info, `Exited 0) ->
begin try Ok (int_of_string v_str) with _ ->
Poster
Owner

match on Failure _

match on `Failure _`
rand marked this conversation as resolved
&&& (fun () -> Option.compare Cstruct.compare
a.hash_debug_bin b.hash_debug_bin)
let of_tuple (uuid, job_name, hash_opam_switch, hash_debug_bin) =
Poster
Owner

Maybe we can write a Caqti_type.custom and use it in builds_vizdeps_q directly.

Maybe we can write a `Caqti_type.custom` and use it in `builds_vizdeps_q` directly.
rand marked this conversation as resolved
| Ok (true, _) -> ()
| Ok (false, path) ->
Logs.warn (fun m ->
m "%s: uuid '%a': Empty file: '%a'"
Poster
Owner

Maybe we can lift this error into check_viz_nonempty

Maybe we can lift this error into `check_viz_nonempty`
rand marked this conversation as resolved
Logs.warn (fun m -> m "verify_completeness_aux: Failure: %a" R.pp_msg err)
| Ok false ->
Logs.warn (fun m ->
m "%s: uuid '%a': Completeness when '.done': Missing file: '%a'"
Poster
Owner

Perhaps we can rephrase this:

"Visualization %s marked as done, but missing visualization for %s uuid %a: '%a'" viz_str build.job_name Uuidm.pp build.uuid Fpath.pp viz_path
Perhaps we can rephrase this: ```OCaml "Visualization %s marked as done, but missing visualization for %s uuid %a: '%a'" viz_str build.job_name Uuidm.pp build.uuid Fpath.pp viz_path ```
Poster
Owner

I had a comment to this about improving this message, but it seems to have gone somewhere...

I had a comment to this about improving this message, but it seems to have gone somewhere...
rand marked this conversation as resolved
aux current_version
let get_viz_version_from_dirs ~cachedir ~viz_typ_str =
let open Rresult.R.Infix in
Poster
Owner

Rresult is being deprecated, you can write let ( >>= ) = Result.bind.

Rresult is being deprecated, you can write `let ( >>= ) = Result.bind`.
rand marked this conversation as resolved
let max_cached_version =
let viz_typ_str = viz_typ_str ^ "_" in
versioned_dirs
|> List.filter_map (fun versioned_dir ->
Poster
Owner

maybe check as the first thing it's a directory and not a file.

maybe check as the first thing it's a directory and not a file.
rand marked this conversation as resolved
rand added 1 commit 2 months ago
rand added 1 commit 2 months ago
rand added 1 commit 2 months ago
rand added 1 commit 2 months ago
rand added 1 commit 2 months ago
rand added 1 commit 2 months ago
rand added 1 commit 2 months ago
rand added 1 commit 2 months ago
reynir changed target branch from automatic-viz-migration to main 2 months ago
Owner

Oh no, I made it conflict hard by changing the target to main :/ I'll see if I can fix it...

Oh no, I made it conflict *hard* by changing the target to `main` :/ I'll see if I can fix it...
Poster
Owner

How come it conflicts when you merged the base branch into main?

How come it conflicts when you merged the base branch into main?
Owner

I added some more commits

I added some more commits
Owner

... though I don't understand why it conflicts in so many places!

... though I don't understand why it conflicts in so many places!
Poster
Owner

Did you force push the #111 PR to main or something? I've also ended up with a bunch of nonsense conflicts each time

Did you force push the #111 PR to main or something? I've also ended up with a bunch of nonsense conflicts each time
Owner

I made a squash merge of #111 to main.

I made a squash merge of #111 to main.
reynir force-pushed 20220524_verify-cache-dir from b22ee88861 to 5234bb5c5a 2 months ago
reynir added 2 commits 2 months ago
d8586b5773 Minor refactoring, check dir existence
a70d7d6da7 Simplify, remove redundant dir existance check
reynir added 1 commit 2 months ago
reynir added 1 commit 2 months ago
886fee19e7 Restructure make_iter__.. and its dependencies
reynir reviewed 2 months ago
&& ending.[0] = '_'
&& string_is_int String.(sub ending 1 (length ending - 1))
in
has_prefix && has_valid_ending
Poster
Owner

Also check that it's a directory?

Also check that it's a directory?
Logs.warn (fun m ->
m "Invalid cache subdirectory name: '%s'" dir_str)
let get_latest_viz_version viz_typ =
Poster
Owner

I am unsure if we should fail if we can't run either modulectomy --version or opam-graph --version with our $PATH.

I am unsure if we should fail if we can't run either `modulectomy --version` or `opam-graph --version` with our `$PATH`.
rand commented 2 months ago
Poster
Owner

Why not fail?

Why not fail?
viz_types |> List.fold_left (fun acc viz_type ->
let viz_prefix = Builder_web.Viz_aux.viz_type_to_string viz_type in
let* acc = acc in
let+ latest_viz_version = get_latest_viz_version viz_type in
Poster
Owner

The motivation for this is to inform the operator if they may need to run batch-viz.sh?
What if we try get_latest_viz_version to get latest version and then check for "completeness" of that dir? And if get_latest_viz_version fails we check for "completeness" with latest version on disk?

I suspect we are still interested about "completeness" of latest version on disk even if it's not the latest according to --version output or if either opam-graph or modulectomy is not in $PATH.

The motivation for this is to inform the operator if they may need to run batch-viz.sh? What if we try `get_latest_viz_version` to get latest version and then check for "completeness" of that dir? And if `get_latest_viz_version` fails we check for "completeness" with latest version on disk? I suspect we are still interested about "completeness" of latest version on disk even if it's not the latest according to `--version` output or if either `opam-graph` or `modulectomy` is not in `$PATH`.
rand commented 2 months ago
Poster
Owner

I believe your suggested semantics (checking completeness of latest version on disk) is what the code did. At least this is the case now, as latest_version is defined by a call to Viz_aux.get_viz_version_from_dirs ~cachedir ~viz_typ.

I think this is the correct semantics, where there then is another check if the latest version of viz-bin has a corresponding cache-dir.

I believe your suggested semantics (checking completeness of latest version on disk) is what the code did. At least this is the case now, as `latest_version` is defined by a call to `Viz_aux.get_viz_version_from_dirs ~cachedir ~viz_typ`. I think this is the correct semantics, where there then is another check if the latest version of viz-bin has a corresponding cache-dir.
Poster
Owner

The behavior as I see it is that verify_cachedir_contents run on the output of {opam-graph,modulectomy} --version. If either command fails we short circuit and print an error in Verify_cache_dir.verify and don't do any further checks. I think it makes sense to check if visualizations have been generated for all builds even if either opam-graph --version or modulectomy --version fails.

The behavior as I see it is that `verify_cachedir_contents` run on the output of `{opam-graph,modulectomy} --version`. If either command fails we short circuit and print an error in `Verify_cache_dir.verify` and don't do any further checks. I think it makes sense to check if visualizations have been generated for all builds even if either `opam-graph --version` or `modulectomy --version` fails.
match build.Build.hash_opam_switch with
| None ->
Logs.warn (fun m ->
m "%s: uuid '%a': Doesn't support dependencies viz because of \
Poster
Owner

If I remember correctly it is still the case that opam-switch doesn't get generated if the job fails. Thus this warning can quickly become noisy.

Perhaps we can detect the build exit status and decide on a lower level if the build exited unsuccessfully. We can do this in a separate PR to get this merged faster.

If I remember correctly it is still the case that `opam-switch` doesn't get generated if the job fails. Thus this warning can quickly become noisy. Perhaps we can detect the build exit status and decide on a lower level if the build exited unsuccessfully. We can do this in a separate PR to get this merged faster.
rand commented 2 months ago
Poster
Owner

I see - yes let's do that in a separate PR

I see - yes let's do that in a separate PR
type msg = [ `Msg of string ]
let open_error_msg : ('a, msg) result -> ('a, [> msg]) result =
Poster
Owner

I don't mind the explicitness at all(!), but it can also be written as let open_error_msg = function Ok _ as r -> r | Error (`Msg _) as r -> r

I don't mind the explicitness at all(!), but it can also be written as `` let open_error_msg = function Ok _ as r -> r | Error (`Msg _) as r -> r``
|> List.filter_map (fun versioned_dir ->
match Bos.OS.Dir.exists versioned_dir with
| Error (`Msg err) ->
Logs.warn (fun m -> m "%s" err);
Poster
Owner

It would be nicer with some context to the warning :)

It would be nicer with some context to the warning :)
reynir merged commit 5307a7b91a into main 2 months ago
hannes deleted branch 20220524_verify-cache-dir 2 months ago
The pull request has been merged as 5307a7b91a.
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.