Opened 10 months ago

Last modified 18 hours ago

#2891 readytorelease defect

Neural network evaluation is not thread-safe

Reported by: gkronber Owned by: gkronber
Priority: medium Milestone: HeuristicLab 3.3.16
Component: Algorithms.DataAnalysis Version: trunk
Keywords: Cc:

Description

It seems the evaluation of neural network models is not thread-safe.

The effects can be observed in the partial dependence plot which uses evaluates a model for different inputs concurrently. The bug leads to unplausible charts.

Change History (11)

comment:1 Changed 10 months ago by gkronber

  • Owner set to gkronber
  • Status changed from new to accepted

comment:2 Changed 10 months ago by gkronber

The processing methods in alglib are indeed not thread-safe

comment:3 Changed 10 months ago by gkronber

  • Owner changed from gkronber to mkommend
  • Status changed from accepted to reviewing

r15739: fixed bug in NN and NN ensemble using a simple lock for processing an input.

comment:4 Changed 10 months ago by gkronber

  • Owner changed from mkommend to pfleck

comment:5 Changed 7 months ago by pfleck

  • Owner changed from pfleck to gkronber

reviewed r15739: Change works, partial dependence plot values looks sensible now (no more random jitter).

Currently, the locks are performed directly on the alglib.multilayerperceptron and alglib.mlpensemble objects. I suggest we use explicit, private lock objects instead, especially because we have a public getter for those alglib objects.

On the other hand, if someone wants to use the alglib objects manually he/she could use the same object for locking even if the object is also used concurrently within our GetEstimated(Class)Values methods. However, I would rather vote for removing the public getter, because it is currently unused and is an implementation detail which should not be public.

comment:6 Changed 6 months ago by gkronber

  • Status changed from reviewing to assigned

comment:7 Changed 3 months ago by gkronber

r16168: incorporated feedback from review, made NeuralNetworkEnsembleModel and NeuralNetworkModel readonly. This is a breaking change to the API

comment:8 Changed 3 months ago by gkronber

  • Owner changed from gkronber to pfleck
  • Status changed from assigned to reviewing

Thanks for the suggestions for improvements. Please review r16168.

comment:9 Changed 3 months ago by abeham

  • Version changed from 3.3.15 to trunk

comment:10 Changed 39 hours ago by pfleck

  • Owner changed from pfleck to gkronber
  • Status changed from reviewing to readytorelease

reviewed r16168: looks good, thanks.

Although making the model readonly (and not publicly accessible) is a breaking API change, I think we can release the ticket since the chance of anyone depending on this is small.

comment:11 Changed 18 hours ago by gkronber

r16387: merged r15739 and r16168 from trunk to stable.

Ticket can be closed if build and unit tests succeed.

Note: See TracTickets for help on using tickets.