| Message ID | 20211125233559.34575-2-zimon.toutoune@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Fix Julia packages. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/applying patch | fail | View Laminar job |
| cbaines/issue | success | View issue |
| cbaines/applying patch | fail | View Laminar job |
| cbaines/issue | success | View issue |
Hello Simon! zimoun <zimon.toutoune@gmail.com> writes: > Even providing '--procs=1' launches 2 workers which breaks some testsuite of ^ the > packages; therefore set '#:parallel-tests?' to '#false' was ineffective. ^ some ^ setting It's good to put the rationale here, as you did. > * 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. > --- > 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? > (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: --8<---------------cut here---------------start------------->8--- (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"))))) --8<---------------cut here---------------start------------->8--- And took the liberty to remove trailing #f in other phases. Thank you! Maxim
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: >> --- >> 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”. --8<---------------cut here---------------start------------->8--- + (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))) + '()) --8<---------------cut here---------------end--------------->8--- So because of 2 tests. I think your modified patch is more “complicated”. :-) About this, --8<---------------cut here---------------start------------->8--- + (additional-procs (max 0 (1- job-count)))) --8<---------------cut here---------------end--------------->8--- 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. 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? Cheers, simon
Hi, On Sat, 27 Nov 2021 at 13:38, zimoun <zimon.toutoune@gmail.com> wrote: > 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
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
Hi Maxim, On Sat, 27 Nov 2021 at 21:57, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote: > 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. > 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: --8<---------------cut here---------------start------------->8--- 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 --8<---------------cut here---------------end--------------->8--- 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
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)