[core-updates-frozen] [PATCH 0/6] Fix Julia packages.

  • Done
  • quality assurance status badge
Details
2 participants
  • Maxim Cournoyer
  • zimoun
Owner
unassigned
Submitted by
zimoun
Severity
normal
Z
Z
zimoun wrote on 26 Nov 2021 00:32
(address . guix-patches@gnu.org)(name . zimoun)(address . zimon.toutoune@gmail.com)
20211125233225.34466-1-zimon.toutoune@gmail.com
Hi,

With this series, all the Julia packages builds again. The only one still
missing is 'julia-sundials-jll’. It probably requires to introduce something
to julia-build-system but I have not investigated much.


Cheers,
simon


zimoun (6):
gnu: julia: Test correctly '#:parallel-tests?'.
build: julia-build-system: Correctly disable parallel tests.
gnu: julia-aqua: Disable parallel tests.
gnu: julia-interpolations: Disable parallel tests.
gnu: julia-requires: Disable parallel tests.
gnu: julia-unitful: Disable parallel tests.

gnu/packages/julia-xyz.scm | 8 ++++++++
gnu/packages/julia.scm | 9 ++++-----
guix/build/julia-build-system.scm | 8 +++++---
3 files changed, 17 insertions(+), 8 deletions(-)


base-commit: 138498feec335f68935f00f8a97924c90c7f59b0
--
2.32.0
Z
Z
zimoun wrote on 26 Nov 2021 00:35
[PATCH 2/6] build: julia-build-system: Correctly disable parallel tests.
(address . 52117@debbugs.gnu.org)(name . zimoun)(address . zimon.toutoune@gmail.com)
20211125233559.34575-2-zimon.toutoune@gmail.com
Even providing '--procs=1' launches 2 workers which breaks some testsuite of
packages; therefore set '#:parallel-tests?' to '#false' was ineffective.

* guix/build/julia-build-system.scm (check): Fix unexpected behaviour from
'julia' command line option.
---
guix/build/julia-build-system.scm | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

Toggle diff (28 lines)
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
(string-append builddir "loadpath/"
package "/test/runtests.jl"))))
#t)
--
2.32.0
Z
Z
zimoun wrote on 26 Nov 2021 00:35
[PATCH 3/6] gnu: julia-aqua: Disable parallel tests.
(address . 52117@debbugs.gnu.org)(name . zimoun)(address . zimon.toutoune@gmail.com)
20211125233559.34575-3-zimon.toutoune@gmail.com
* gnu/packages/julia-xyz.scm (julia-aqua)[arguments]: Disable parallel tests.
---
gnu/packages/julia-xyz.scm | 2 ++
1 file changed, 2 insertions(+)

Toggle diff (15 lines)
diff --git a/gnu/packages/julia-xyz.scm b/gnu/packages/julia-xyz.scm
index 547d2bf81e..3ca32097e9 100644
--- a/gnu/packages/julia-xyz.scm
+++ b/gnu/packages/julia-xyz.scm
@@ -136,6 +136,8 @@ (define-public julia-aqua
(sha256
(base32 "1g0kyzcdykgs247j72jpc2qqall696jwgb3hnn4cxmbi8bkf7wpk"))))
(build-system julia-build-system)
+ (arguments
+ `(#:parallel-tests? #f))
(home-page "https://github.com/JuliaTesting/Aqua.jl")
(synopsis "Automated quality assurance for Julia packages")
(description "@acronym{Aqua.jl, Auto QUality Assurance for Julia packages},
--
2.32.0
Z
Z
zimoun wrote on 26 Nov 2021 00:35
[PATCH 1/6] gnu: julia: Test correctly '#:parallel-tests?'.
(address . 52117@debbugs.gnu.org)(name . zimoun)(address . zimon.toutoune@gmail.com)
20211125233559.34575-1-zimon.toutoune@gmail.com
* gnu/packages/julia.scm (julia)[arguments]<#:phases>: Fix parallel-test.
---
gnu/packages/julia.scm | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)

Toggle diff (22 lines)
diff --git a/gnu/packages/julia.scm b/gnu/packages/julia.scm
index ac5bf7db25..c123711da5 100644
--- a/gnu/packages/julia.scm
+++ b/gnu/packages/julia.scm
@@ -315,11 +315,10 @@ (define-public julia
(substitute* (jlpath "Zlib")
(((from "libz")) (to "zlib" "libz"))))))
(add-after 'unpack 'enable-parallel-tests
- ;; FIXME: julia fails at networking in the container and falls back
- ;; to a single worker, which causes the tests to not run in
- ;; parallel (see: https://github.com/JuliaLang/julia/issues/43205).
- (lambda* (#:key parallel-build? #:allow-other-keys)
- (setenv "JULIA_CPU_THREADS" (if parallel-build?
+ ;; Parallel tests require 'julia-allow-parallel-build.patch'.
+ ;; https://github.com/JuliaLang/julia/issues/43205.
+ (lambda* (#:key parallel-tests? #:allow-other-keys)
+ (setenv "JULIA_CPU_THREADS" (if parallel-tests?
(number->string (parallel-job-count))
"1"))
(format #t "JULIA_CPU_THREADS environment variable set to ~a~%"
--
2.32.0
Z
Z
zimoun wrote on 26 Nov 2021 00:35
[PATCH 4/6] gnu: julia-interpolations: Disable parallel tests.
(address . 52117@debbugs.gnu.org)(name . zimoun)(address . zimon.toutoune@gmail.com)
20211125233559.34575-4-zimon.toutoune@gmail.com
* gnu/packages/julia-xyz.scm (julia-interpolations)[arguments]: Disable
parallel tests.
---
gnu/packages/julia-xyz.scm | 2 ++
1 file changed, 2 insertions(+)

Toggle diff (15 lines)
diff --git a/gnu/packages/julia-xyz.scm b/gnu/packages/julia-xyz.scm
index 3ca32097e9..f68cd1647b 100644
--- a/gnu/packages/julia-xyz.scm
+++ b/gnu/packages/julia-xyz.scm
@@ -2518,6 +2518,8 @@ (define-public julia-interpolations
(sha256
(base32 "1236c20k388qlh7k74mhf7hkbn0vf7ss8b1rgh1a6aj0234ayfnc"))))
(build-system julia-build-system)
+ (arguments
+ `(#:parallel-tests? #f))
(propagated-inputs
`(("julia-axisalgorithms" ,julia-axisalgorithms)
("julia-offsetarrays" ,julia-offsetarrays)
--
2.32.0
Z
Z
zimoun wrote on 26 Nov 2021 00:35
[PATCH 5/6] gnu: julia-requires: Disable parallel tests.
(address . 52117@debbugs.gnu.org)(name . zimoun)(address . zimon.toutoune@gmail.com)
20211125233559.34575-5-zimon.toutoune@gmail.com
* gnu/packages/julia-xyz.scm (julia-requires)[arguments]: Disable parallel
tests.
---
gnu/packages/julia-xyz.scm | 2 ++
1 file changed, 2 insertions(+)

Toggle diff (15 lines)
diff --git a/gnu/packages/julia-xyz.scm b/gnu/packages/julia-xyz.scm
index f68cd1647b..6ddf1429ca 100644
--- a/gnu/packages/julia-xyz.scm
+++ b/gnu/packages/julia-xyz.scm
@@ -3966,6 +3966,8 @@ (define-public julia-requires
(sha256
(base32 "03hyfy7c0ma45b0y756j76awi3az2ii4bz4s8cxm3xw9yy1z7b01"))))
(build-system julia-build-system)
+ (arguments
+ `(#:parallel-tests? #f))
(inputs ;required for test
`(("julia-example" ,julia-example)))
(propagated-inputs
--
2.32.0
Z
Z
zimoun wrote on 26 Nov 2021 00:35
[PATCH 6/6] gnu: julia-unitful: Disable parallel tests.
(address . 52117@debbugs.gnu.org)(name . zimoun)(address . zimon.toutoune@gmail.com)
20211125233559.34575-6-zimon.toutoune@gmail.com
* gnu/packages/julia-xyz.scm (julia-unitful)[arguments]: Disable parallel
tests.
---
gnu/packages/julia-xyz.scm | 2 ++
1 file changed, 2 insertions(+)

Toggle diff (15 lines)
diff --git a/gnu/packages/julia-xyz.scm b/gnu/packages/julia-xyz.scm
index 6ddf1429ca..d2e57aeadc 100644
--- a/gnu/packages/julia-xyz.scm
+++ b/gnu/packages/julia-xyz.scm
@@ -4854,6 +4854,8 @@ (define-public julia-unitful
(sha256
(base32 "10qwscd15dnmvx116dwvg99m7kmwgmj5ahdkq7psiq48lcc554gq"))))
(build-system julia-build-system)
+ (arguments
+ `(#:parallel-tests? #f))
(propagated-inputs
`(("julia-constructionbase" ,julia-constructionbase)))
(home-page "https://painterqubits.github.io/Unitful.jl/stable/")
--
2.32.0
M
M
Maxim Cournoyer wrote on 27 Nov 2021 03:26
Re: bug#52117: [core-updates-frozen] [PATCH 0/6] Fix Julia packages.
(name . zimoun)(address . zimon.toutoune@gmail.com)(address . 52117@debbugs.gnu.org)
87k0gu8hby.fsf_-_@gmail.com
Hello Simon!

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

Toggle quote (26 lines)
> * gnu/packages/julia.scm (julia)[arguments]<#:phases>: Fix parallel-test.
> ---
> gnu/packages/julia.scm | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/gnu/packages/julia.scm b/gnu/packages/julia.scm
> index ac5bf7db25..c123711da5 100644
> --- a/gnu/packages/julia.scm
> +++ b/gnu/packages/julia.scm
> @@ -315,11 +315,10 @@ (define-public julia
> (substitute* (jlpath "Zlib")
> (((from "libz")) (to "zlib" "libz"))))))
> (add-after 'unpack 'enable-parallel-tests
> - ;; FIXME: julia fails at networking in the container and falls back
> - ;; to a single worker, which causes the tests to not run in
> - ;; parallel (see: https://github.com/JuliaLang/julia/issues/43205).
> - (lambda* (#:key parallel-build? #:allow-other-keys)
> - (setenv "JULIA_CPU_THREADS" (if parallel-build?
> + ;; Parallel tests require 'julia-allow-parallel-build.patch'.
> + ;; https://github.com/JuliaLang/julia/issues/43205.
> + (lambda* (#:key parallel-tests? #:allow-other-keys)
> + (setenv "JULIA_CPU_THREADS" (if parallel-tests?
> (number->string (parallel-job-count))
> "1"))
> (format #t "JULIA_CPU_THREADS environment variable set to ~a~%"

I've moved the comment where I thought it made more sense, at the top of
the patch itself, like so:

Toggle snippet (38 lines)
modified gnu/packages/julia.scm
@@ -315,8 +315,6 @@ (define-public julia
(substitute* (jlpath "Zlib")
(((from "libz")) (to "zlib" "libz"))))))
(add-after 'unpack 'enable-parallel-tests
- ;; Parallel tests require 'julia-allow-parallel-build.patch'.
- ;; https://github.com/JuliaLang/julia/issues/43205.
(lambda* (#:key parallel-tests? #:allow-other-keys)
(setenv "JULIA_CPU_THREADS" (if parallel-tests?
(number->string (parallel-job-count))
modified gnu/packages/patches/julia-allow-parallel-build.patch
@@ -1,3 +1,8 @@
+Allow parallel tests with isolated environment.
+
+See https://github.com/JuliaLang/julia/issues/43205 and
+https://github.com/JuliaLang/julia/pull/43211.
+
diff --git a/test/runtests.jl b/test/runtests.jl
index 2f9cd058bb..150395e78c 100644
--- a/test/runtests.jl
@@ -11,14 +16,11 @@ index 2f9cd058bb..150395e78c 100644
using Base: Experimental
include("choosetests.jl")
-@@ -83,11 +83,15 @@ prepend!(tests, linalg_tests)
+@@ -83,11 +83,12 @@ prepend!(tests, linalg_tests)
import LinearAlgebra
cd(@__DIR__) do
n = 1
- if net_on
-+ # Allow parallel tests with isolated environment
-+ # https://github.com/JuliaLang/julia/issues/43205
-+ # https://github.com/JuliaLang/julia/pull/43211
+ if net_on || haskey(ENV, "JULIA_CPU_THREADS")
n = min(Sys.CPU_THREADS, length(tests))
n > 1 && addprocs_with_testenv(n)

Maxim
M
M
Maxim Cournoyer wrote on 27 Nov 2021 04:17
(name . zimoun)(address . zimon.toutoune@gmail.com)(address . 52117@debbugs.gnu.org)
87fsri8ez5.fsf_-_@gmail.com
Hello Simon!

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

Toggle quote (1 lines)
> Even providing '--procs=1' launches 2 workers which breaks some testsuite of
^ the
Toggle quote (1 lines)
> packages; therefore set '#:parallel-tests?' to '#false' was ineffective.
^ some ^ setting

It's good to put the rationale here, as you did.

Toggle quote (3 lines)
> * guix/build/julia-build-system.scm (check): Fix unexpected behaviour from
> 'julia' command line option.

But in the changelog message I'd expect to see foremost *what* it does
rather than a reformulation of *why* it does it :-). E.g., something
like: do not pass the '--procs' argument when not running the tests in
parallel.

Toggle quote (28 lines)
> ---
> 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?

Toggle quote (4 lines)
> (string-append builddir "loadpath/"
> package "/test/runtests.jl"))))
> #t)

Trailing '#t' are no longer required. Actually, looking at the output
of julia --help:

-p, --procs {N|auto} Integer value N launches N *additional* local worker processes

The key is 'additional' :-). So to disable parallel processing it needs
to be 0.

I've modified it like so:

Toggle snippet (28 lines)
(define* (check #:key tests? source inputs outputs julia-package-name
parallel-tests? #:allow-other-keys)
(when tests?
(let* ((out (assoc-ref outputs "out"))
(package (or julia-package-name (project.toml->name "Project.toml")))
(builddir (string-append out "/share/julia/"))
(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.
(additional-procs (max 0 (1- job-count))))
;; With a patch, SOURCE_DATE_EPOCH is honored
(setenv "SOURCE_DATE_EPOCH" "1")
(setenv "JULIA_DEPOT_PATH" builddir)
(setenv "JULIA_LOAD_PATH"
(string-append builddir "loadpath/" ":"
(or (getenv "JULIA_LOAD_PATH")
"")))
(setenv "JULIA_CPU_THREADS" (number->string job-count))
(setenv "HOME" "/tmp")
(invoke "julia" "--depwarn=yes"
"--procs" (number->string additional-procs)
(string-append builddir "loadpath/"
package "/test/runtests.jl")))))

And took the liberty to remove trailing #f in other phases.

Thank you!

Maxim
M
M
Maxim Cournoyer wrote on 27 Nov 2021 07:31
Re: [bug#52117] [core-updates-frozen] [PATCH 0/6] Fix Julia packages.
(name . zimoun)(address . zimon.toutoune@gmail.com)(address . 52117-done@debbugs.gnu.org)
8735ni85yw.fsf@gmail.com
Hello Simon,

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

Toggle quote (27 lines)
> Hi,
>
> With this series, all the Julia packages builds again. The only one still
> missing is 'julia-sundials-jll’. It probably requires to introduce something
> to julia-build-system but I have not investigated much.
>
>
> Cheers,
> simon
>
>
> zimoun (6):
> gnu: julia: Test correctly '#:parallel-tests?'.
> build: julia-build-system: Correctly disable parallel tests.
> gnu: julia-aqua: Disable parallel tests.
> gnu: julia-interpolations: Disable parallel tests.
> gnu: julia-requires: Disable parallel tests.
> gnu: julia-unitful: Disable parallel tests.
>
> gnu/packages/julia-xyz.scm | 8 ++++++++
> gnu/packages/julia.scm | 9 ++++-----
> guix/build/julia-build-system.scm | 8 +++++---
> 3 files changed, 17 insertions(+), 8 deletions(-)
>
>
> base-commit: 138498feec335f68935f00f8a97924c90c7f59b0

Now pushed to core-updates-frozen; I've made some changes to the
build-system one after finding out that --procs meant "adding" cores,
rather than specifying their exact count (and you cannot add 0!).

The parallel build of Julia packages uses a ton of RAM, but it's fast!
:-).

Thanks a lot,

Maxim
Closed
Z
Z
zimoun wrote on 27 Nov 2021 13:38
Re: bug#52117: [core-updates-frozen] [PATCH 0/6] Fix Julia packages.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 52117@debbugs.gnu.org)
861r31kc2m.fsf@gmail.com
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. :-)


On Fri, 26 Nov 2021 at 22:17, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

Toggle quote (31 lines)
>> ---
>> 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”.

Toggle snippet (19 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”. :-)


About this,

Toggle snippet (3 lines)
+ (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.


Last, I am confused by Cuirass. Because it says evaluation complete but
julia-* packages are scheduled.


And for instance,


BTW, Berlin has some issues I guess

#48720 - d508c5b was pushed CommitDate: Fri Nov 26 23:21:45 2021 +0100
- 941f776 was pushed CommitDate: Sat Nov 27 01:22:32 2021 -0500
- 9c4752b was pushed CommitDate: Sat Nov 27 10:24:12 2021 +0100
#48802 - 1b8a18b was pushed CommitDate: Sat Nov 27 11:48:17 2021 +0100

I do not understand why 941f776 or 9c4752b had not been evaluated.

Could you give a look? For example, by restarting the evaluation?


Cheers,
simon
Z
Z
zimoun wrote on 27 Nov 2021 19:59
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 52117@debbugs.gnu.org)
86sfvhifw6.fsf@gmail.com
Hi,

On Sat, 27 Nov 2021 at 13:38, zimoun <zimon.toutoune@gmail.com> wrote:

Toggle quote (20 lines)
> Last, I am confused by Cuirass. Because it says evaluation complete but
> julia-* packages are scheduled.
>
> https://ci.guix.gnu.org/eval/48802?status=pending
>
> And for instance,
>
> https://ci.guix.gnu.org/build/1853818/log/raw
>
> BTW, Berlin has some issues I guess
>
> #48720 - d508c5b was pushed CommitDate: Fri Nov 26 23:21:45 2021 +0100
> - 941f776 was pushed CommitDate: Sat Nov 27 01:22:32 2021 -0500
> - 9c4752b was pushed CommitDate: Sat Nov 27 10:24:12 2021 +0100
> #48802 - 1b8a18b was pushed CommitDate: Sat Nov 27 11:48:17 2021 +0100
>
> I do not understand why 941f776 or 9c4752b had not been evaluated.
>
> Could you give a look? For example, by restarting the evaluation?

Re-checking now, all is green. \o/
I am still confused by Cuirass but that’s another story, I guess.


Thanks for helping in this yak shaving. :-)

Cheers,
simon
M
M
Maxim Cournoyer wrote on 28 Nov 2021 03:57
(name . zimoun)(address . zimon.toutoune@gmail.com)(address . 52117@debbugs.gnu.org)
87tufx6l74.fsf@gmail.com
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
Z
Z
zimoun wrote on 29 Nov 2021 15:10
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 52117@debbugs.gnu.org)
87tufvqchy.fsf_-_@gmail.com
Hi Maxim,

On Sat, 27 Nov 2021 at 21:57, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

Toggle quote (14 lines)
> 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:
>
> (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/'

Thanks for the explanations.


Toggle quote (4 lines)
> 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 :-).

Indeed, and it is expected to fail because:

Toggle snippet (13 lines)
def _get_main_module_details(error=ImportError):
# Helper that gives a nicer error message when attempting to
# execute a zipfile or directory by invoking __main__.py
main_name = "__main__"
try:
return _get_module_details(main_name)
except ImportError as exc:
if main_name in str(exc):
raise error("can't find %r module in %r" %
(main_name, sys.path[0]))
raise

It allows to do:

$ mkdir /tmp/foo
$ echo print(42) > /tmp/foo/__main__.py
$ python /tmp/foo

Therefore, this

$ python '' -c '0'

just fails. Contrary to,

$ cd /tmp/foo
$ python '' -c '0'

which just passes. To me, it is an oddity of the Python command-line
which silently accepts a path; it is not documented by “python -h”.

Anyway, I agree that the behaviour when passing "" is up to the
application, therefore it should be avoided.


Cheers,
simon
?