all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
To: zimoun <zimon.toutoune@gmail.com>
Cc: 52117@debbugs.gnu.org
Subject: [bug#52117] [core-updates-frozen] [PATCH 0/6] Fix Julia packages.
Date: Sat, 27 Nov 2021 21:57:51 -0500	[thread overview]
Message-ID: <87tufx6l74.fsf@gmail.com> (raw)
In-Reply-To: <861r31kc2m.fsf@gmail.com> (zimoun's message of "Sat, 27 Nov 2021 13:38:57 +0100")

Hello Simon,

zimoun <zimon.toutoune@gmail.com> writes:

> Hi Maxim,
>
> Thanks for the review and the improved patch.
>
> I am sorry if the commit message and/or changelog I provided was badly
> worded, but somehow it was an attempt to explain the odd behaviour – at
> least counter-intuitive since I initially felt into when sending the
> very first patch allowing parallel tests and you felt too, I guess. :-)

No worries.  Communicating changes (or anything) is always one of the
greatest challenges in programming and elsewhere, it seems :-).  The
nice thing about it is that it can be improved with perseverance and
some feedback.

>
> On Fri, 26 Nov 2021 at 22:17, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
>>> ---
>>>  guix/build/julia-build-system.scm | 8 +++++---
>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/guix/build/julia-build-system.scm b/guix/build/julia-build-system.scm
>>> index f0dc419c17..af478fd4a3 100644
>>> --- a/guix/build/julia-build-system.scm
>>> +++ b/guix/build/julia-build-system.scm
>>> @@ -112,7 +112,10 @@ (define* (check #:key tests? source inputs outputs julia-package-name
>>>             (builddir (string-append out "/share/julia/"))
>>>             (jobs (if parallel-tests?
>>>                       (number->string (parallel-job-count))
>>> -                     "1")))
>>> +                     "1"))
>>> +           (nprocs (if parallel-tests?
>>> +                       (string-append "--procs=" jobs)
>>> +                       "")))
>>>        ;; With a patch, SOURCE_DATE_EPOCH is honored
>>>        (setenv "SOURCE_DATE_EPOCH" "1")
>>>        (setenv "JULIA_DEPOT_PATH" builddir)
>>> @@ -122,8 +125,7 @@ (define* (check #:key tests? source inputs outputs julia-package-name
>>>                                   "")))
>>>        (setenv "JULIA_CPU_THREADS" jobs)
>>>        (setenv "HOME" "/tmp")
>>> -      (invoke "julia" "--depwarn=yes"
>>> -              (string-append "--procs=" jobs)
>>> +      (invoke "julia" "--depwarn=yes" nprocs
>>
>> Here nprocs can be ""; is it really OK to pass an empty string argument
>> to julia?
>
> Yes it is OK.  When #:parallel-tests? sets to #f, my patch leads to
> the call “julia --depwarn=yes” which is valid.  Your modified patch
> adds another test but leads to the same call “julia --depwarn=yes”.

No, it would invoke julia with the following argv list: "julia"
"-depwarn=yes" "" [...];

My point is that invoke is equivalent to doing an execlp system call;
and the arguments get passed as a list (including that empty string
argument when parallel-tests? is #f).  Whether this works or not is up
to the application, so I'd suggest not relying on it.  Consider for
example:

--8<---------------cut here---------------start------------->8---
(execlp "python" "python" "" "-c" "print('hello')")
/gnu/store/cwqv4z5bvb5x6i0zvqgc1j1dnr6w9vp8-profile/bin/python: can't
find '__main__' module in
'/home/maxim/src/guix-core-updates-next/gnu/packages/'
--8<---------------cut here---------------end--------------->8---

It fails because it interprets the empty string argument as the current
directory, apparently.  If that works with the above Julia invocation,
that's great, but it doesn't make it cleaner in my opinion :-).

> +           (job-count (if parallel-tests?
> +                          (parallel-job-count)
> +                          1))
> +           ;; The --proc argument of Julia *adds* extra processors rather than
> +           ;; specify the exact count to use, so zero must be specified to
> +           ;; disable parallel processing...
>
> [..]
>
> +      (apply invoke "julia"
> +             `("--depwarn=yes"
> +               ,@(if parallel-tests?
> +                     ;; XXX: ... but '--procs' doesn't accept 0 as a valid
> +                     ;; value, so just omit the argument entirely.
> +                     (list (string-append  "--procs="
> +                                           (number->string additional-procs)))
> +                     '())
>
>
> So because of 2 tests. I think your modified patch is more
> “complicated”. :-)

It is slightly more complex indeed; but I think it provides the reader
with useful knowledge of julia's quirks and is more correct.

> About this,
>
> +           (additional-procs (max 0 (1- job-count))))
>
> I considered that it was not a big deal; initially, I did something
> similar in ’let’ but remove it because it changes nothing from my
> experiments.  In fact, because ’--procs’ overrides JULIA_CPU_THREADS and
> run Julia with n or n+1 is more or less the same for the Julia land,
> IMHO.  Well, it is not clear what is the load for the main worker. And
> JULIA_CPU_THREADS=1 is required for running using only one worker.
> Anyway, this changes nothing, practically speaking. :-) Indeed, it is
> better and more consistent.

Yeah, I don't like that the behavior of --procs is to *add* workers,
rather than set its exact count; which make thing awkward.  Even
upstream get tricked by that by erroneously *adding* Sys.CPU_THREADS
workers because of this in test/runtest.jl [0]

[0]  https://github.com/JuliaLang/julia/pull/43217#pullrequestreview-817102530

Thanks,

Maxim




  parent reply	other threads:[~2021-11-28  2:58 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-25 23:32 [bug#52117] [core-updates-frozen] [PATCH 0/6] Fix Julia packages zimoun
2021-11-25 23:35 ` [bug#52117] [PATCH 1/6] gnu: julia: Test correctly '#:parallel-tests?' zimoun
2021-11-25 23:35   ` [bug#52117] [PATCH 2/6] build: julia-build-system: Correctly disable parallel tests zimoun
2021-11-27  3:17     ` [bug#52117] [core-updates-frozen] [PATCH 0/6] Fix Julia packages Maxim Cournoyer
2021-11-27 12:38       ` zimoun
2021-11-27 18:59         ` zimoun
2021-11-28  2:57         ` Maxim Cournoyer [this message]
2021-11-29 14:10           ` zimoun
2021-11-25 23:35   ` [bug#52117] [PATCH 3/6] gnu: julia-aqua: Disable parallel tests zimoun
2021-11-25 23:35   ` [bug#52117] [PATCH 4/6] gnu: julia-interpolations: " zimoun
2021-11-25 23:35   ` [bug#52117] [PATCH 5/6] gnu: julia-requires: " zimoun
2021-11-25 23:35   ` [bug#52117] [PATCH 6/6] gnu: julia-unitful: " zimoun
2021-11-27  2:26   ` [bug#52117] [core-updates-frozen] [PATCH 0/6] Fix Julia packages Maxim Cournoyer
2021-11-27  6:31 ` bug#52117: " Maxim Cournoyer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87tufx6l74.fsf@gmail.com \
    --to=maxim.cournoyer@gmail.com \
    --cc=52117@debbugs.gnu.org \
    --cc=zimon.toutoune@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/guix.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.