GNU bug report logs - #32300
Cuirass: the 'nr' filter doesn't work when builds have multiple outputs

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 32300 in the body.
You can then email your comments to 32300 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Clément Lassieur <clement <at> lassieur.org>
To: bug-guix <at> gnu.org
Subject: Cuirass: the 'nr' filter doesn't when builds have multiple outputs
Date: Sun, 29 Jul 2018 01:21:10 +0200

Hi,

With, say, 'nr' = 4, the GROUP-OUTPUTS procedure in cuirass/database.scm
will transform

  0 | out   | /gnu/store/...
  1 | out   | /gnu/store/...
  1 | debug | /gnu/store/...
  2 | out   | /gnu/store/...

into

  ((#:id . 0) (#:outputs ("out"   (#:path . "/gnu/store/..."))))
  ((#:id . 1) (#:outputs ("out"   (#:path . "/gnu/store/..."))
                         ("debug" (#:path . "/gnu/store/..."))))
  ((#:id . 2) (#:outputs ("out"   (#:path . "/gnu/store/..."))))

Thus there are only 3 elements returned by the low-level DB-GET-BUILDS
procedure, while we expect 4.

This bug is visible through the API (latestbuilds, queue) and the web
interface (eval) because they use that DB-GET-BUILDS procedure.

Clément

Message #10 received at 32300 <at> debbugs.gnu.org (full text, mbox):

From: Clément Lassieur <clement <at> lassieur.org>
To: 32300 <at> debbugs.gnu.org
Subject: Re: bug#32300: Cuirass: the 'nr' filter doesn't work when builds have
 multiple outputs
Date: Sun, 29 Jul 2018 01:26:32 +0200

Typo in subject: it doesn't *work.

Message #13 received at 32300 <at> debbugs.gnu.org (full text, mbox):

From: Clément Lassieur <clement <at> lassieur.org>
To: 32300 <at> debbugs.gnu.org
Subject: [PATCH] database: Fix the builds limit issue.
Date: Sat,  4 Aug 2018 18:00:57 +0200

Fixes <https://bugs.gnu.org/32300>.

* src/cuirass/database.scm (filters->order): New procedure.
(db-get-builds): Remove FORMAT-OUTPUT, CONS-OUTPUT, COLLECT-OUTPUTS,
FINISH-GROUP, SAME-GROUP?, GROUP-OUTPUTS procedures.  Remove the 'LEFT JOIN
Outputs' clause.  Use DB-GET-OUTPUTS for each build that was fetched.
---
 src/cuirass/database.scm | 126 ++++++++++++---------------------------
 1 file changed, 37 insertions(+), 89 deletions(-)

diff --git a/src/cuirass/database.scm b/src/cuirass/database.scm
index 4927f2a..b4b1652 100644
--- a/src/cuirass/database.scm
+++ b/src/cuirass/database.scm
@@ -443,104 +443,33 @@ log file for DRV."
              (cons `(,name . ((#:path . ,path)))
                    outputs))))))
 
+(define (filters->order filters)
+  (match (assq 'order filters)
+    (('order . 'build-id) "id ASC")
+    (('order . 'decreasing-build-id) "id DESC")
+    (('order . 'finish-time) "stoptime DESC")
+    (('order . 'finish-time+build-id) "stoptime DESC, id DESC")
+    (('order . 'start-time) "starttime DESC")
+    (('order . 'submission-time) "timestamp DESC")
+    ;; With this order, builds in 'running' state (-1) appear
+    ;; before those in 'scheduled' state (-2).
+    (('order . 'status+submission-time) "status DESC, timestamp DESC")
+    (_ "id DESC")))
+
 (define (db-get-builds db filters)
   "Retrieve all builds in database DB which are matched by given FILTERS.
 FILTERS is an assoc list whose possible keys are 'id | 'jobset | 'job |
 'system | 'nr | 'order | 'status | 'evaluation."
-
-  (define (format-output name path)
-    `(,name . ((#:path . ,path))))
-
-  (define (cons-output name path rest)
-    "If NAME and PATH are both not #f, cons them to REST.
-Otherwise return REST unchanged."
-    (if (and (not name) (not path))
-        rest
-        (cons (format-output name path) rest)))
-
-  (define (collect-outputs repeated-builds-id repeated-row outputs rows)
-    "Given rows somewhat like
-1 'a 'b  2 'x
-^ 'c 'd  2 'x
-| ^^^^^  ^^^^
-| group  ++++- group headers
-| detail
-+------------ group id
-
-return rows somewhat like
-
-1 2 'x '((a b) (c d))
-
-.
-
-As a special case, if the group detail is #f #f, ignore it.
-This is made specifically to support LEFT JOINs.
-
-Assumes that if group id stays the same the group headers stay the same."
-    (define (finish-group)
-      (match repeated-row
-        (#(timestamp starttime stoptime log status derivation job-name system
-                     nix-name specification)
-         `((#:id         . ,repeated-builds-id)
-           (#:timestamp  . ,timestamp)
-           (#:starttime  . ,starttime)
-           (#:stoptime   . ,stoptime)
-           (#:log        . ,log)
-           (#:status     . ,status)
-           (#:derivation . ,derivation)
-           (#:job-name   . ,job-name)
-           (#:system     . ,system)
-           (#:nix-name   . ,nix-name)
-           (#:specification . ,specification)
-           (#:outputs    . ,outputs)))))
-
-    (define (same-group? builds-id)
-      (= builds-id repeated-builds-id))
-
-    (match rows
-      (() (list (finish-group)))
-      ((#((? same-group? x-builds-id) x-output-name x-output-path other-cells ...) . rest)
-       ;; Accumulate group members of current group.
-       (let ((outputs (cons-output x-output-name x-output-path outputs)))
-         (collect-outputs repeated-builds-id repeated-row outputs rest)))
-      ((#(x-builds-id x-output-name x-output-path other-cells ...) . rest)
-       (cons (finish-group)                       ;finish current group
-
-             ;; Start new group.
-             (let* ((outputs (cons-output x-output-name x-output-path '()))
-                    (x-repeated-row (list->vector other-cells)))
-               (collect-outputs x-builds-id x-repeated-row outputs rest))))))
-
-  (define (group-outputs rows)
-    (match rows
-      (() '())
-      ((#(x-builds-id x-output-name x-output-path other-cells ...) . rest)
-       (let ((x-repeated-row (list->vector other-cells)))
-         (collect-outputs x-builds-id x-repeated-row '() rows)))))
-
-  (let* ((order (match (assq 'order filters)
-                  (('order . 'build-id) "id ASC")
-                  (('order . 'decreasing-build-id) "id DESC")
-                  (('order . 'finish-time) "stoptime DESC")
-                  (('order . 'finish-time+build-id) "stoptime DESC, id DESC")
-                  (('order . 'start-time) "starttime DESC")
-                  (('order . 'submission-time) "timestamp DESC")
-                  (('order . 'status+submission-time)
-                   ;; With this order, builds in 'running' state (-1) appear
-                   ;; before those in 'scheduled' state (-2).
-                   "status DESC, timestamp DESC")
-                  (_ "id DESC")))
+  (let* ((order (filters->order filters))
          (stmt-text (format #f "SELECT * FROM (
-SELECT Builds.id, Outputs.name, Outputs.path, Builds.timestamp,
-Builds.starttime, Builds.stoptime, Builds.log, Builds.status,
-Builds.derivation, Derivations.job_name, Derivations.system,
-Derivations.nix_name,Specifications.name
+SELECT Builds.id, Builds.timestamp, Builds.starttime, Builds.stoptime,
+Builds.log, Builds.status, Builds.derivation, Derivations.job_name,
+Derivations.system, Derivations.nix_name, Specifications.name
 FROM Builds
 INNER JOIN Derivations ON Builds.derivation = Derivations.derivation
 AND Builds.evaluation = Derivations.evaluation
 INNER JOIN Evaluations ON Derivations.evaluation = Evaluations.id
 INNER JOIN Specifications ON Evaluations.specification = Specifications.name
-LEFT JOIN Outputs ON Outputs.build = Builds.id
 WHERE (:id IS NULL OR (:id = Builds.id))
 AND (:jobset IS NULL OR (:jobset = Specifications.name))
 AND (:job IS NULL OR (:job = Derivations.job_name))
@@ -580,7 +509,26 @@ ORDER BY ~a, id ASC;" order))
             (#f -1)
             (x x)))
     (sqlite-reset stmt)
-    (group-outputs (sqlite-fold-right cons '() stmt))))
+    (let loop ((rows (sqlite-fold-right cons '() stmt))
+               (builds '()))
+      (match rows
+        (() (reverse builds))
+        ((#(id timestamp starttime stoptime log status derivation job-name
+               system nix-name specification) . rest)
+         (loop rest
+               (cons `((#:id . ,id)
+                       (#:timestamp . ,timestamp)
+                       (#:starttime . ,starttime)
+                       (#:stoptime . ,stoptime)
+                       (#:log . ,log)
+                       (#:status . ,status)
+                       (#:derivation . ,derivation)
+                       (#:job-name . ,job-name)
+                       (#:system . ,system)
+                       (#:nix-name . ,nix-name)
+                       (#:specification . ,specification)
+                       (#:outputs . ,(db-get-outputs db id)))
+                     builds)))))))
 
 (define (db-get-build db id)
   "Retrieve a build in database DB which corresponds to ID."
-- 
2.18.0

Message #16 received at 32300 <at> debbugs.gnu.org (full text, mbox):

From: Clément Lassieur <clement <at> lassieur.org>
To: 32300 <at> debbugs.gnu.org
Subject: Re: bug#32300: [PATCH] database: Fix the builds limit issue.
Date: Sat, 04 Aug 2018 18:10:51 +0200

Clément Lassieur <clement <at> lassieur.org> writes:

> Fixes <https://bugs.gnu.org/32300>.
>
> * src/cuirass/database.scm (filters->order): New procedure.
> (db-get-builds): Remove FORMAT-OUTPUT, CONS-OUTPUT, COLLECT-OUTPUTS,
> FINISH-GROUP, SAME-GROUP?, GROUP-OUTPUTS procedures.  Remove the 'LEFT JOIN
> Outputs' clause.  Use DB-GET-OUTPUTS for each build that was fetched.

This may be less efficient because there are more SQL queries (one per
output), but it's way less complicated and less buggy, so I think it's
worth it.

Message #19 received at 32300 <at> debbugs.gnu.org (full text, mbox):

From: Danny Milosavljevic <dannym <at> scratchpost.org>
To: Clément Lassieur <clement <at> lassieur.org>
Cc: 32300 <at> debbugs.gnu.org
Subject: Re: bug#32300: [PATCH] database: Fix the builds limit issue.
Date: Tue, 7 Aug 2018 12:46:26 +0200

[Message part 1 (text/plain, inline)]

Hi Clément,

On Sat, 04 Aug 2018 18:10:51 +0200
Clément Lassieur <clement <at> lassieur.org> wrote:

> Clément Lassieur <clement <at> lassieur.org> writes:
> 
> > Fixes <https://bugs.gnu.org/32300>.
> >
> > * src/cuirass/database.scm (filters->order): New procedure.
> > (db-get-builds): Remove FORMAT-OUTPUT, CONS-OUTPUT, COLLECT-OUTPUTS,
> > FINISH-GROUP, SAME-GROUP?, GROUP-OUTPUTS procedures.  Remove the 'LEFT JOIN
> > Outputs' clause.  Use DB-GET-OUTPUTS for each build that was fetched.  
> 
> This may be less efficient because there are more SQL queries (one per
> output), but it's way less complicated and less buggy, so I think it's
> worth it.

The more complicated version is a LOT faster - and was added because
the version in this patch was just way too slow (unusably slow).

I think it's better to also remove the call to db-get-outputs (and
the entry #:outputs) entirely.  I don't think our overview page even
shows the outputs in the first place, so why fetch them?

[Message part 2 (application/pgp-signature, inline)]

Message #22 received at 32300 <at> debbugs.gnu.org (full text, mbox):

From: Clément Lassieur <clement <at> lassieur.org>
To: Danny Milosavljevic <dannym <at> scratchpost.org>
Cc: 32300 <at> debbugs.gnu.org
Subject: Re: bug#32300: [PATCH] database: Fix the builds limit issue.
Date: Wed, 08 Aug 2018 13:44:44 +0200

Hi Danny,

Danny Milosavljevic <dannym <at> scratchpost.org> writes:

> The more complicated version is a LOT faster - and was added because
> the version in this patch was just way too slow (unusably slow).
>
> I think it's better to also remove the call to db-get-outputs (and
> the entry #:outputs) entirely.  I don't think our overview page even
> shows the outputs in the first place, so why fetch them?

As you can see on my test machine with a Berlin database, it's almost
instantaneous.  Also, with my commit that merges the Derivations and the
Builds tables, the queries are way lighter: they return about 100 builds
per evaluation, instead of 20000.

Plus, the outputs are used to get the build log.

https://cuirass.lassieur.org:8081/jobset/guix-master

WDYT?
Clément

Message #25 received at 32300 <at> debbugs.gnu.org (full text, mbox):

From: Clément Lassieur <clement <at> lassieur.org>
To: Danny Milosavljevic <dannym <at> scratchpost.org>,
 Tatiana Sholokhova <tanja201396 <at> gmail.com>
Cc: 32300 <at> debbugs.gnu.org
Subject: Re: bug#32300: [PATCH] database: Fix the builds limit issue.
Date: Wed, 08 Aug 2018 17:32:06 +0200

Clément Lassieur <clement <at> lassieur.org> writes:

> Hi Danny,
>
> Danny Milosavljevic <dannym <at> scratchpost.org> writes:
>
>> The more complicated version is a LOT faster - and was added because
>> the version in this patch was just way too slow (unusably slow).
>>
>> I think it's better to also remove the call to db-get-outputs (and
>> the entry #:outputs) entirely.  I don't think our overview page even
>> shows the outputs in the first place, so why fetch them?
>
> As you can see on my test machine with a Berlin database, it's almost
> instantaneous.  Also, with my commit that merges the Derivations and the
> Builds tables, the queries are way lighter: they return about 100 builds
> per evaluation, instead of 20000.
>
> Plus, the outputs are used to get the build log.
>
> https://cuirass.lassieur.org:8081/jobset/guix-master

I just reverted to my own Cuirass config because my hard drive is too
small (256G...) to build everything.

And I added Tatiana's recent changes.

Clément

Message #28 received at 32300 <at> debbugs.gnu.org (full text, mbox):

From: Danny Milosavljevic <dannym <at> scratchpost.org>
To: Clément Lassieur <clement <at> lassieur.org>
Cc: 32300 <at> debbugs.gnu.org
Subject: Re: bug#32300: [PATCH] database: Fix the builds limit issue.
Date: Thu, 9 Aug 2018 07:57:09 +0200

[Message part 1 (text/plain, inline)]

Hi,

On Sat, 04 Aug 2018 18:10:51 +0200
Clément Lassieur <clement <at> lassieur.org> wrote:

> Clément Lassieur <clement <at> lassieur.org> writes:
> 
> > Fixes <https://bugs.gnu.org/32300>.
> >
> > * src/cuirass/database.scm (filters->order): New procedure.
> > (db-get-builds): Remove FORMAT-OUTPUT, CONS-OUTPUT, COLLECT-OUTPUTS,
> > FINISH-GROUP, SAME-GROUP?, GROUP-OUTPUTS procedures.  Remove the 'LEFT JOIN
> > Outputs' clause.  Use DB-GET-OUTPUTS for each build that was fetched.  
> 
> This may be less efficient because there are more SQL queries (one per
> output), but it's way less complicated and less buggy, so I think it's
> worth it.

Yeah, if it's still usable, I agree.

But I think we shouldn't overlook the possibility of not fetching the outputs
in the first place (at all).

[Message part 2 (application/pgp-signature, inline)]

Message #31 received at 32300 <at> debbugs.gnu.org (full text, mbox):

From: Clément Lassieur <clement <at> lassieur.org>
To: Danny Milosavljevic <dannym <at> scratchpost.org>
Cc: 32300 <at> debbugs.gnu.org
Subject: Re: bug#32300: [PATCH] database: Fix the builds limit issue.
Date: Thu, 09 Aug 2018 10:02:47 +0200

Danny Milosavljevic <dannym <at> scratchpost.org> writes:

> Hi,
>
> On Sat, 04 Aug 2018 18:10:51 +0200
> Clément Lassieur <clement <at> lassieur.org> wrote:
>
>> Clément Lassieur <clement <at> lassieur.org> writes:
>> 
>> > Fixes <https://bugs.gnu.org/32300>.
>> >
>> > * src/cuirass/database.scm (filters->order): New procedure.
>> > (db-get-builds): Remove FORMAT-OUTPUT, CONS-OUTPUT, COLLECT-OUTPUTS,
>> > FINISH-GROUP, SAME-GROUP?, GROUP-OUTPUTS procedures.  Remove the 'LEFT JOIN
>> > Outputs' clause.  Use DB-GET-OUTPUTS for each build that was fetched.  
>> 
>> This may be less efficient because there are more SQL queries (one per
>> output), but it's way less complicated and less buggy, so I think it's
>> worth it.
>
> Yeah, if it's still usable, I agree.
>
> But I think we shouldn't overlook the possibility of not fetching the outputs
> in the first place (at all).

I totally agree!  Although it should be another commit, I think.

Message #36 received at 32300-done <at> debbugs.gnu.org (full text, mbox):

From: Clément Lassieur <clement <at> lassieur.org>
To: Danny Milosavljevic <dannym <at> scratchpost.org>
Cc: 32300-done <at> debbugs.gnu.org
Subject: Re: bug#32300: [PATCH] database: Fix the builds limit issue.
Date: Thu, 16 Aug 2018 22:59:39 +0200

> Hi,
>
> On Sat, 04 Aug 2018 18:10:51 +0200
> Clément Lassieur <clement <at> lassieur.org> wrote:
>
>> Clément Lassieur <clement <at> lassieur.org> writes:
>> 
>> > Fixes <https://bugs.gnu.org/32300>.
>> >
>> > * src/cuirass/database.scm (filters->order): New procedure.
>> > (db-get-builds): Remove FORMAT-OUTPUT, CONS-OUTPUT, COLLECT-OUTPUTS,
>> > FINISH-GROUP, SAME-GROUP?, GROUP-OUTPUTS procedures.  Remove the 'LEFT JOIN
>> > Outputs' clause.  Use DB-GET-OUTPUTS for each build that was fetched.  
>> 
>> This may be less efficient because there are more SQL queries (one per
>> output), but it's way less complicated and less buggy, so I think it's
>> worth it.
>
> Yeah, if it's still usable, I agree.
>
> But I think we shouldn't overlook the possibility of not fetching the outputs
> in the first place (at all).

Pushed.  Thank you for the review!

Clément

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.