-
Notifications
You must be signed in to change notification settings - Fork 184
[bugfix, enhancement] Address affinity bug by using threadpoolctl/joblib for n_jobs dispatching #2364
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
base: main
Are you sure you want to change the base?
[bugfix, enhancement] Address affinity bug by using threadpoolctl/joblib for n_jobs dispatching #2364
Changes from all commits
c78859f
e207110
043b09d
66b0b6d
bc66055
280c0e0
eb0df7f
2403e6d
009348b
29c318f
ed726de
2b58453
78d07bb
8da0891
79ced00
e021335
30f822a
6d02aea
84f91ac
a2a499a
ac16042
e6fdd80
04075dc
dd798fa
70d613e
1b121a5
7bd1fcb
bbb2337
a3ceaf3
97a906f
1948e7d
62c7d9f
ce79ace
8765c0a
e2fa126
1ca56cd
ab1c1eb
e9b5da5
f979da3
b56729e
2b2749c
3f0155b
385ad80
052bcdd
c638473
e00feb3
627df75
3bf60b5
df38221
51ccef3
17de438
8da3d2e
4be9afc
1a117df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -114,7 +114,7 @@ def gen_functions(functions): | |
|
||
data_shapes = [ | ||
pytest.param((1000, 100), id="(1000, 100)"), | ||
pytest.param((2000, 50), id="(2000, 50)"), | ||
pytest.param((2000, 40), id="(2000, 40)"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change alone makes memory usage test failures. @icfaust, what is the reason for it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My goal was to see if I could tweak the parameters to make it either completely disappear or be consistent, giving a way to properly address the issue. Is this something you can now reproduce locally? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test failures disappear locally if data shape change is reverted. Not sure if it will be the same in CI. |
||
] | ||
|
||
EXTRA_MEMORY_THRESHOLD = 0.15 | ||
|
Uh oh!
There was an error while loading. Please reload this page.
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 understand this setting would apply globally, which could lead to race conditions if users call this in parallel, for example through some framework that would parallelize estimator calls.
Could it somehow get a mutex (or use atomic ops) either here or on the oneDAL side?
Also, would be better to add a warning that the setting is changed at a global level, so that a user would not try to call these inside multi-threaded code.
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.
Actually on a further look, it does already have a mutex on the daal side. Still better to document this behavior being global.
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.
Sounds good, will do!