Changed Pkg.test to prioritise julia_args --threads over current process threads #4141
Conversation
…ess threads Ensures that tests run with threads, both when calling `julia --project --threads=auto --eval 'using Pkg; Pkg.test()'`, and when calling `julia --project --eval 'using Pkg; Pkg.test(; julia_args = ["--threads=auto"])'`.
|
Makes sense. Can you add a test |
|
Not completely sure how to properly add this test, but what I am aiming for is something similar to running the following for the added EXPECTED_NTHREADS=1 julia +nightly --project --eval 'using Pkg; Pkg.test()'
EXPECTED_NTHREADS=2 julia +nightly --project --threads=2 --eval 'using Pkg; Pkg.test()'
EXPECTED_NTHREADS=2 julia +nightly --project --eval 'using Pkg; Pkg.test(; julia_args=["--threads=2"])'
EXPECTED_NTHREADS=1 julia +nightly --project --threads=2 --eval 'using Pkg; Pkg.test(; julia_args=["--threads=1"])'Edit: This is the behaviour on Julia 1.6, and 1.10. |
|
I guess, one could also add: |
|
Any suggestions for how to implement the tests? |
|
Take a look at existing test packages and add one that can be called by |
|
Just a passing comment that this should probably account for and test both threadpools |
|
Did something change post-Julia-1.11, that remedied the issue with passing |
src/Operations.jl
Outdated
| flush(ctx.io) | ||
| code = gen_test_code(source_path; test_args) | ||
| cmd = `$(Base.julia_cmd()) $(flags) --eval $code` | ||
| cmd = Cmd(`$(Base.julia_cmd()) $(flags) --eval $code`; env = julia_args.env) |
There was a problem hiding this comment.
What about the other parts of the julia_args Cmd?
Is it safe to ignore julia_args.cpus, julia_args.dir, julia_args.flags (!= flags), and julia_args.ignorestatus?
stemann
force-pushed
the
feature/test_julia_args_threads
branch
from
ad66721 to
304fffd
Compare
|
Is this passable? |
src/Operations.jl
Outdated
| cmd = `$(Base.julia_cmd()) $(flags) --threads=$(get_threads_spec()) --eval $code` | ||
| julia_args_defines_threads = any(startswith("--threads"), julia_args.exec) || (!isnothing(julia_args.env) && any(startswith("JULIA_NUM_THREADS"), julia_args.env)) | ||
| threads_arg = julia_args_defines_threads ? `` : `--threads=$(get_threads_spec())` | ||
| cmd = Cmd(`$(Base.julia_cmd()) $threads_arg $(flags) --eval $code`; env = julia_args.env) |
There was a problem hiding this comment.
This feels like a lot more change than is needed.
Don't we just need to move --threads to before $flags?
So
cmd = `$(Base.julia_cmd()) --threads=$(get_threads_spec()) $(flags) --eval $code`
i.e.
% ./julia -t2 -t3 -E "Threads.nthreads(:default)"
3
Then the testing can be done with a normal
withenv(...) do
Pkg.test(....)
end
There was a problem hiding this comment.
I believe I needed to also add support for passing through the environment variables to the test process in order to implement the tests...
... it could possibly be split into two PRs...
There was a problem hiding this comment.
But the last commit ("Added @test try catch end to count Pkg.test invocations as tests) could be reverted...
There was a problem hiding this comment.
… but yeah, the withenv approach might be sufficient…
There was a problem hiding this comment.
With 7aa3a80, without the modification of the environment passed to the test process via julia_args, the tests attempting to set threads via JULIA_NUM_THREADS fail.
…ess threads (JuliaLang#4141) Co-authored-by: Ian Butterworth <[email protected]> (cherry picked from commit f517489)
…ess threads (#4141) Co-authored-by: Ian Butterworth <[email protected]> (cherry picked from commit f517489)
…ess threads (#4141) Co-authored-by: Ian Butterworth <[email protected]> (cherry picked from commit f517489)
3 participants
Changed Pkg.test to prioritise julia_args --threads over current process threads
Ensures that tests run with threads, both when calling
julia --project --threads=auto --eval 'using Pkg; Pkg.test()', and when callingjulia --project --eval 'using Pkg; Pkg.test(; julia_args = ["--threads=auto"])'.Similar to #4140