[MRG+1] FIX out of bounds array access in SAGA #9376

jnothman · 2017-07-16T13:49:48Z

Fixes #9351 (random inconsistencies between SAGA and liblinear LogisticRegression on OS X).

To hedge my bets that this is the right fix, I'll ping @TomDLT, @arthurmensch, @dsullivan7.

agramfort · 2017-07-16T14:02:42Z

did you spot this by setting boundscheck=True?

jnothman · 2017-07-16T14:03:38Z

I did not. I found it by narrowing in using old-school diffs over printed debug output.

jnothman · 2017-07-16T14:04:38Z

Is there a way to disable the nogil feature for a compilation so that exceptions will be propagated?

jnothman · 2017-07-16T14:04:55Z

I commented on the boundscheck at #9351 (comment)

agramfort · 2017-07-16T14:06:49Z

I confirm that I don't get the error in the branch after running 10 times the test. It fails on master on my mac.

jnothman · 2017-07-16T14:08:06Z

I've tried it well over 10 times, if that adds any weight. :)

And even disabling the nogil, I'm not getting an exception raised with boundscheck=True :\

ogrisel · 2017-07-16T18:18:46Z

To get the exceptions to propagate:

change the prototype of:

cdef void lagged_update(...) nogil:

to:

cdef int lagged_update(...) nogil except -1:

and update the implementation to return 0.

wrap the code that raises the exception with "with gil":

with gil:
    raise ValueError(the_msg)

ogrisel · 2017-07-16T19:37:45Z

I also confirm that fixes the issue on my mac. The diff looks good, however I am not familiar enough with the algorithm to tell whether the fix is correct. @arthurmensch can you confirm?

jnothman · 2017-07-16T21:27:12Z

Well, I'm sure that the successful runs are those when cumulative_sums[-1] happened to be 0. Which is why I would get behaviours like an entire run of 1000 trials would succeed, then a run of 1000 trials would fail 1/3 times (due to whatever memory the process was allocated and, I presume, garbage collection, because in this case I found it was precisely every third run that failed).

jnothman · 2017-07-16T23:40:27Z

If there's reason to believe @arthurmensch will take more than a day to review, I'd consider merging this as a temporary fix and building the RC. It does, after all, make saga equal liblinear.

arthurmensch · 2017-07-17T05:14:10Z

I will be reviewing this in one hour

…

On Mon, Jul 17, 2017, 1:40 AM Joel Nothman ***@***.***> wrote: If there's reason to believe @arthurmensch will take more than a day to review, I'd consider merging this as a temporary fix and building the RC. It does, after all, make saga equal liblinear. On 17 Jul 2017 5:37 am, "Olivier Grisel" ***@***.***> wrote: I also confirm that fixes the issue on my mac. The diff looks good, however I am not familiar enough with the algorithm to tell whether the fix is correct. @arthurmensch <https://github.com/arthurmensch> can you confirm? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub < #9376 (comment) >, or mute the thread < https://github.com/notifications/unsubscribe-auth/AAEz6yS7EsHt_SWILOwNqhE5ask1PCb3ks5sOmaLgaJpZM4OZTV_ > . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9376 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD48YyXK3fsSKaGFbXLF9xvqow4hlPqoks5sOp91gaJpZM4OZTV_> .

jnothman · 2017-07-17T05:24:42Z

:D

…

On 17 Jul 2017 3:14 pm, "Arthur Mensch" ***@***.***> wrote: I will be reviewing this in one hour On Mon, Jul 17, 2017, 1:40 AM Joel Nothman ***@***.***> wrote: > If there's reason to believe @arthurmensch will take more than a day to > review, I'd consider merging this as a temporary fix and building the RC. > It does, after all, make saga equal liblinear. > > On 17 Jul 2017 5:37 am, "Olivier Grisel" ***@***.***> wrote: > > I also confirm that fixes the issue on my mac. The diff looks good, however > I am not familiar enough with the algorithm to tell whether the fix is > correct. @arthurmensch <https://github.com/arthurmensch> can you confirm? > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > < > #9376# issuecomment-315631765 > >, > or mute the thread > < > https://github.com/notifications/unsubscribe-auth/AAEz6yS7EsHt_ SWILOwNqhE5ask1PCb3ks5sOmaLgaJpZM4OZTV_ > > > . > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#9376# issuecomment-315646390>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ AD48YyXK3fsSKaGFbXLF9xvqow4hlPqoks5sOp91gaJpZM4OZTV_> > . > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9376 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz62WY3X8aREydHX9GdtZ52AsqXLCBks5sOu2lgaJpZM4OZTV_> .

arthurmensch · 2017-07-17T06:43:38Z

sklearn/linear_model/sag_fast.pyx

-                           - cumulative_sums[lagged_ind - 1])
-                        prox_step = (cumulative_sums_prox[lagged_ind]
-                           - cumulative_sums_prox[lagged_ind - 1])
+                        if lagged_ind > 1:


I think it should read >= 1

Yes! Whoops. I think we were all a bit too enthusiastic to fix this.

arthurmensch · 2017-07-17T06:44:42Z

sklearn/linear_model/sag_fast.pyx

-                           - cumulative_sums[lagged_ind - 1])
-                        prox_step = (cumulative_sums_prox[lagged_ind]
-                           - cumulative_sums_prox[lagged_ind - 1])
+                        if lagged_ind > 1:


Otherwise there was indeed a problem there, I wonder how it went silently although wraparound = False, any idea ?

Right: those are pointers.

The wraparound, etc, flags do not apply at all to pointers.

But the basic issue is that O/Ses allocate memory on the heap differently. Most of it tends to be zero (if only because we use 64 bit int arrays to store small numbers). So this just worked when the array happened to be allocated next to a zero ... which always happened on Travis and AppVeyor.

Yes. We choose pointers because apparently they're a little more efficient than memoryviews, and much better where we want to handle read-only memmaps. But they're much less safe!

jnothman · 2017-07-17T06:54:01Z

Fixed. Good for you?

arthurmensch · 2017-07-17T06:59:51Z

LGTM. I hope there is no other errors like this... I wondered why we used pointer there in the first place, but it's true that it allows to handle read-only memmap. Thanks !

ogrisel · 2017-07-17T08:49:52Z

Thanks very much for the fix, merging.

ogrisel · 2017-07-17T08:50:14Z

I will do the backport to 0.19.X.

* FIX out of bounds array access in SAGA * FIX Fix boundary for condition

ogrisel · 2017-07-17T08:56:25Z

Done.

Release 0.19b2 * tag '0.19b2': (808 commits) Preparing 0.19b2 [MRG+1] FIX out of bounds array access in SAGA (scikit-learn#9376) FIX make test_importances pass on 32 bit linux Release 0.19b1 DOC remove 'in dev' header in whats_new.rst DOC typos in whats_news.rst [ci skip] [MRG] DOC cleaning up what's new for 0.19 (scikit-learn#9252) FIX t-SNE memory usage and many other optimizer issues (scikit-learn#9032) FIX broken link in gallery and bad title rendering [MRG] DOC Replace \acute by prime (scikit-learn#9332) Fix typos (scikit-learn#9320) [MRG + 1 (rv) + 1 (alex) + 1] Add a check to test the docstring params and their order (scikit-learn#9206) DOC Residual sum vs. regression sum (scikit-learn#9314) [MRG] [HOTFIX] Fix capitalization in test and hence fix failing travis at master (scikit-learn#9317) More informative error message for classification metrics given regression output (scikit-learn#9275) [MRG] COSMIT Remove unused parameters in private functions (scikit-learn#9310) [MRG+1] Ridgecv normalize (scikit-learn#9302) [MRG + 2] ENH Allow `cross_val_score`, `GridSearchCV` et al. to evaluate on multiple metrics (scikit-learn#7388) Add data_home parameter to fetch_kddcup99 (scikit-learn#9289) FIX makedirs(..., exists_ok) not available in Python 2 (scikit-learn#9284) ...

* releases: (808 commits) Preparing 0.19b2 [MRG+1] FIX out of bounds array access in SAGA (scikit-learn#9376) FIX make test_importances pass on 32 bit linux Release 0.19b1 DOC remove 'in dev' header in whats_new.rst DOC typos in whats_news.rst [ci skip] [MRG] DOC cleaning up what's new for 0.19 (scikit-learn#9252) FIX t-SNE memory usage and many other optimizer issues (scikit-learn#9032) FIX broken link in gallery and bad title rendering [MRG] DOC Replace \acute by prime (scikit-learn#9332) Fix typos (scikit-learn#9320) [MRG + 1 (rv) + 1 (alex) + 1] Add a check to test the docstring params and their order (scikit-learn#9206) DOC Residual sum vs. regression sum (scikit-learn#9314) [MRG] [HOTFIX] Fix capitalization in test and hence fix failing travis at master (scikit-learn#9317) More informative error message for classification metrics given regression output (scikit-learn#9275) [MRG] COSMIT Remove unused parameters in private functions (scikit-learn#9310) [MRG+1] Ridgecv normalize (scikit-learn#9302) [MRG + 2] ENH Allow `cross_val_score`, `GridSearchCV` et al. to evaluate on multiple metrics (scikit-learn#7388) Add data_home parameter to fetch_kddcup99 (scikit-learn#9289) FIX makedirs(..., exists_ok) not available in Python 2 (scikit-learn#9284) ...

* dfsg: (808 commits) Preparing 0.19b2 [MRG+1] FIX out of bounds array access in SAGA (scikit-learn#9376) FIX make test_importances pass on 32 bit linux Release 0.19b1 DOC remove 'in dev' header in whats_new.rst DOC typos in whats_news.rst [ci skip] [MRG] DOC cleaning up what's new for 0.19 (scikit-learn#9252) FIX t-SNE memory usage and many other optimizer issues (scikit-learn#9032) FIX broken link in gallery and bad title rendering [MRG] DOC Replace \acute by prime (scikit-learn#9332) Fix typos (scikit-learn#9320) [MRG + 1 (rv) + 1 (alex) + 1] Add a check to test the docstring params and their order (scikit-learn#9206) DOC Residual sum vs. regression sum (scikit-learn#9314) [MRG] [HOTFIX] Fix capitalization in test and hence fix failing travis at master (scikit-learn#9317) More informative error message for classification metrics given regression output (scikit-learn#9275) [MRG] COSMIT Remove unused parameters in private functions (scikit-learn#9310) [MRG+1] Ridgecv normalize (scikit-learn#9302) [MRG + 2] ENH Allow `cross_val_score`, `GridSearchCV` et al. to evaluate on multiple metrics (scikit-learn#7388) Add data_home parameter to fetch_kddcup99 (scikit-learn#9289) FIX makedirs(..., exists_ok) not available in Python 2 (scikit-learn#9284) ...

* FIX out of bounds array access in SAGA * FIX Fix boundary for condition

FIX out of bounds array access in SAGA

db22f5f

jnothman added Blocker Bug labels Jul 16, 2017

jnothman added this to the 0.19 milestone Jul 16, 2017

jnothman mentioned this pull request Jul 16, 2017

Unstable LogisticRegression with saga solver l1 penalty under macOS #9351

Closed

jnothman requested a review from ogrisel July 16, 2017 13:50

agramfort changed the title ~~[MRG] FIX out of bounds array access in SAGA~~ [MRG+1] FIX out of bounds array access in SAGA Jul 16, 2017

arthurmensch reviewed Jul 17, 2017

View reviewed changes

FIX Fix boundary for condition

1bc4f82

ogrisel merged commit 055d17b into scikit-learn:master Jul 17, 2017

ogrisel pushed a commit that referenced this pull request Jul 17, 2017

[MRG+1] FIX out of bounds array access in SAGA (#9376)

0ec2275

* FIX out of bounds array access in SAGA * FIX Fix boundary for condition

TomDLT mentioned this pull request Jul 17, 2017

Flaky tests in logistic regression #8879

Closed

dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017

[MRG+1] FIX out of bounds array access in SAGA (scikit-learn#9376)

df908df

* FIX out of bounds array access in SAGA * FIX Fix boundary for condition

dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017

[MRG+1] FIX out of bounds array access in SAGA (scikit-learn#9376)

612e146

* FIX out of bounds array access in SAGA * FIX Fix boundary for condition

NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017

[MRG+1] FIX out of bounds array access in SAGA (scikit-learn#9376)

6f8ceb6

* FIX out of bounds array access in SAGA * FIX Fix boundary for condition

paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

[MRG+1] FIX out of bounds array access in SAGA (scikit-learn#9376)

0c0281a

* FIX out of bounds array access in SAGA * FIX Fix boundary for condition

AishwaryaRK pushed a commit to AishwaryaRK/scikit-learn that referenced this pull request Aug 29, 2017

[MRG+1] FIX out of bounds array access in SAGA (scikit-learn#9376)

ee26527

* FIX out of bounds array access in SAGA * FIX Fix boundary for condition

maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

[MRG+1] FIX out of bounds array access in SAGA (scikit-learn#9376)

55d0559

* FIX out of bounds array access in SAGA * FIX Fix boundary for condition

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

[MRG+1] FIX out of bounds array access in SAGA (scikit-learn#9376)

6b5de61

* FIX out of bounds array access in SAGA * FIX Fix boundary for condition

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+1] FIX out of bounds array access in SAGA #9376

[MRG+1] FIX out of bounds array access in SAGA #9376

jnothman commented Jul 16, 2017 •

edited

Loading

agramfort commented Jul 16, 2017

jnothman commented Jul 16, 2017

jnothman commented Jul 16, 2017

jnothman commented Jul 16, 2017

agramfort commented Jul 16, 2017

jnothman commented Jul 16, 2017

ogrisel commented Jul 16, 2017 •

edited

Loading

ogrisel commented Jul 16, 2017

jnothman commented Jul 16, 2017

jnothman commented Jul 16, 2017 via email •

edited by TomDLT

Loading

arthurmensch commented Jul 17, 2017 via email

jnothman commented Jul 17, 2017 via email

arthurmensch Jul 17, 2017

jnothman Jul 17, 2017

arthurmensch Jul 17, 2017

arthurmensch Jul 17, 2017

jnothman Jul 17, 2017

jnothman Jul 17, 2017

jnothman commented Jul 17, 2017

arthurmensch commented Jul 17, 2017

ogrisel commented Jul 17, 2017

ogrisel commented Jul 17, 2017

ogrisel commented Jul 17, 2017

[MRG+1] FIX out of bounds array access in SAGA #9376

[MRG+1] FIX out of bounds array access in SAGA #9376

Conversation

jnothman commented Jul 16, 2017 • edited Loading

agramfort commented Jul 16, 2017

jnothman commented Jul 16, 2017

jnothman commented Jul 16, 2017

jnothman commented Jul 16, 2017

agramfort commented Jul 16, 2017

jnothman commented Jul 16, 2017

ogrisel commented Jul 16, 2017 • edited Loading

ogrisel commented Jul 16, 2017

jnothman commented Jul 16, 2017

jnothman commented Jul 16, 2017 via email • edited by TomDLT Loading

arthurmensch commented Jul 17, 2017 via email

jnothman commented Jul 17, 2017 via email

arthurmensch Jul 17, 2017

Choose a reason for hiding this comment

jnothman Jul 17, 2017

Choose a reason for hiding this comment

arthurmensch Jul 17, 2017

Choose a reason for hiding this comment

arthurmensch Jul 17, 2017

Choose a reason for hiding this comment

jnothman Jul 17, 2017

Choose a reason for hiding this comment

jnothman Jul 17, 2017

Choose a reason for hiding this comment

jnothman commented Jul 17, 2017

arthurmensch commented Jul 17, 2017

ogrisel commented Jul 17, 2017

ogrisel commented Jul 17, 2017

ogrisel commented Jul 17, 2017

jnothman commented Jul 16, 2017 •

edited

Loading

ogrisel commented Jul 16, 2017 •

edited

Loading

jnothman commented Jul 16, 2017 via email •

edited by TomDLT

Loading