Hello Simon,
zimoun <zimon.toutoune@gmail.com> writes:
Toggle quote (9 lines)
> 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.
Toggle quote (38 lines)
>
> 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:
Toggle snippet (6 lines)
(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/'
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 :-).
Toggle quote (22 lines)
> + (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.
Toggle quote (13 lines)
> 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]
Thanks,
Maxim
