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
Ensure a prescribed threading layer can load in CI. #7606
Conversation
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
buildscripts/incremental/test.sh
Outdated
cmd="import os;\ | ||
from numba.misc.numba_sysinfo import get_sysinfo;\ | ||
print(get_sysinfo()['$1 Threading'])" | ||
python -c "$cmd" |
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.
Would an eval
on the result of the look and followed by an assert
in the string command be as clear as the additional "check result of query" in bash? Would save having to store the print into a bash var and test it later.
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.
an uncaught exception should cause the python process to exit 1
. Or you can also call os.exit(1)
.
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.
Indeed, I'll give this a try to limit mixing two languages.
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.
@stuartarchibald, do you want to make this change?
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.
Done in 1b4666b. Now uses assert
.
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.
Thanks for the patch. I have a few comments.
buildscripts/incremental/test.sh
Outdated
cmd="import os;\ | ||
from numba.misc.numba_sysinfo import get_sysinfo;\ | ||
print(get_sysinfo()['$1 Threading'])" | ||
python -c "$cmd" |
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.
an uncaught exception should cause the python process to exit 1
. Or you can also call os.exit(1)
.
buildscripts/incremental/test.sh
Outdated
|
||
if [[ "$TEST_THREADING" ]]; then | ||
if [[ "$TEST_THREADING" == "tbb" ]]; then | ||
tstat=$(check_sysinfo "TBB") |
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 find it hard to tell if this is running or not. Can use more echo
when it passes
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've updated the python snippet to print on success in 1b4666b
Shouldn't this fail w/o #7605? |
As title
bcf1dc2 forces the TBB version check |
Seemingly unrelated fail in linux, python 3.7, np1.18 with coverage:
Link to log: https://dev.azure.com/numba/ff1fe4d0-ed73-4f1c-b894-1d50a27e048f/_apis/build/builds/10403/logs/94 |
I can reproduce the above locally:
Then install Numba merging this branch with mainline. Then do:
the hit rate for the error is unfortunately pretty low. |
Cannot reproduce the issue on mailine (in 20 runs), I'd guess the problem on this branch is caused by some state from loading |
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's now a conflict and a small comment:
function check_sysinfo() { | ||
cmd="import os;\ | ||
from numba.misc.numba_sysinfo import get_sysinfo;\ | ||
assert get_sysinfo()['$1 Threading'] is True, 'Threading layer $1 '\ |
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.
Does this really need to be an identity check?
assert get_sysinfo()['$1 Threading'] is True, 'Threading layer $1 '\ | |
assert get_sysinfo()['$1 Threading'], 'Threading layer $1 '\ |
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 about this when writing it, I don't think there's any enforced return type for the output of get_sysinfo
as it's largely informational only. My concern was that someone might patch this output to return a string like "It's working" or "It's not working", at which point a boolean assert would alias. If you feel differently am happy to change.
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.
fair. we can leave it as is
Conflicts: numba/misc/numba_sysinfo.py
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.
Thanks for the patch!
As title