-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
For running tests, use up to 32 cores if available instead of 8. #18302
Conversation
@@ -26,7 +26,7 @@ max_worker_rss != typemax(Csize_t) && move_to_node1("parallel") | |||
cd(dirname(@__FILE__)) do | |||
n = 1 | |||
if net_on | |||
n = min(8, Sys.CPU_CORES, length(tests)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a maximum at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. I just made the smallest change. I wonder if there are systems with lots of cores and too little memory. If anything, it would be better to have an environment variable to change this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do.... JULIA_CPU_CORES
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will that set Sys.CPU_CORES and hence also change the number of tests run simultaneously? If so, we should just make this min(Sys.CPU_CORES, length(tests))
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there are systems with lots of cores and too little memory
FWIW, HiKey board can have 8 cores and 1G of memory (the situation is common for ARM boards), which is likely not enough to run the test (even with a low JULIA_TEST_MAX_RSS
) on all cores but an arbitrary worker number limit won't help with this either......
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tkelman Do you know why it is the way it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably sth related to julia.mit.edu? It's before JULIA_CPU_CORES
was added so I suppose any similar needs can be addressed with the env now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the idea was to avoid accidentally overloading a shared system when typing make test
and being a bad citizen there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps people doing that should use a command line parameter? Or alternately, there should be an easier way to use more cores for tests.
…and number of tests.
e9c5c1a
to
458db49
Compare
I've made this |
I am merging this for now, and we can tweak this further as necessary. |
we should probably add an overrideable limit in the makefile so we default to not going too crazy with this |
I have tested with up to 48 cores being used simultaneously for tests. If the system has fewer cores, then the test system anyways picks up fewer. This lets us use more cores if available, which I find quite handy on larger systems.