Skip to content

Convert LdaEngine to a SafeHandle #4538

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

Merged
merged 1 commit into from
Dec 9, 2019

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented Dec 7, 2019

This pull request follows the test failure observed in https://dev.azure.com/dnceng/public/_build/results?buildId=449759&view=ms.vss-test-web.build-test-results-tab&runId=14347330&resultId=100770&paneView=debug. Converting this object to a SafeHandle has two primary advantages:

  1. The object will not be disposed while there is an ongoing native call
  2. Misuse of the object by trying to pass an invalid instance to a native call will throw an exception at the point of the call

@sharwell sharwell requested a review from a team as a code owner December 7, 2019 00:02
Copy link
Member

@codemzs codemzs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@sharwell sharwell requested a review from codemzs December 7, 2019 01:59
@codecov
Copy link

codecov bot commented Dec 7, 2019

Codecov Report

Merging #4538 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4538      +/-   ##
==========================================
+ Coverage   75.12%   75.12%   +<.01%     
==========================================
  Files         908      908              
  Lines      160214   160220       +6     
  Branches    17250    17250              
==========================================
+ Hits       120355   120365      +10     
+ Misses      35045    35039       -6     
- Partials     4814     4816       +2
Flag Coverage Δ
#Debug 75.12% <100%> (ø) ⬆️
#production 70.52% <100%> (ø) ⬆️
#test 90.31% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
src/Microsoft.ML.Transforms/Text/LdaSingleBox.cs 63.63% <100%> (+1.13%) ⬆️
....ML.AutoML/PipelineSuggesters/PipelineSuggester.cs 79.83% <0%> (-3.37%) ⬇️
...rosoft.ML.AutoML/ColumnInference/TextFileSample.cs 59.6% <0%> (-2.65%) ⬇️
...soft.ML.TestFramework/DataPipe/TestDataPipeBase.cs 76.08% <0%> (-0.4%) ⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs 86.26% <0%> (+0.15%) ⬆️
src/Microsoft.ML.AutoML/Sweepers/ISweeper.cs 67.08% <0%> (+7.59%) ⬆️
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs 100% <0%> (+20.51%) ⬆️

@sharwell sharwell changed the title Convert LdaEngine to a SafeHandle WIP Convert LdaEngine to a SafeHandle Dec 8, 2019
@sharwell

This comment has been minimized.

@sharwell sharwell force-pushed the safe-lda-engine-handle branch from 0cbfa54 to bd45716 Compare December 9, 2019 07:15
@sharwell sharwell changed the title WIP Convert LdaEngine to a SafeHandle Convert LdaEngine to a SafeHandle Dec 9, 2019
@sharwell
Copy link
Member Author

sharwell commented Dec 9, 2019

With changes to the native library now merged (#4551), I reduced this pull request to the minimum necessary to stabilize the use of this particular handle. It specifically fixes cases where this exception appears in test runs:

  Error Message:
   System.InvalidOperationException : Splitter/consolidator worker encountered exception while consuming source data
---- System.AccessViolationException : Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
  Stack Trace:
     at Microsoft.ML.Data.DataViewUtils.Splitter.Batch.SetAll(OutPipe[] pipes) in D:\a\1\s\src\Microsoft.ML.Data\Data\DataViewUtils.cs:line 832
   at Microsoft.ML.Data.DataViewUtils.Splitter.Cursor.MoveNextCore() in D:\a\1\s\src\Microsoft.ML.Data\Data\DataViewUtils.cs:line 1102
   at Microsoft.ML.Data.RootCursorBase.MoveNext() in D:\a\1\s\src\Microsoft.ML.Core\Data\RootCursorBase.cs:line 65
   at Microsoft.ML.RunTests.TestDataPipeNoBaseline.TestLDATransform() in D:\a\1\s\test\Microsoft.ML.TestFramework\DataPipe\TestDataPipe.cs:line 1335
----- Inner Stack Trace -----
   at Microsoft.ML.TextAnalytics.LdaInterface.InitializeBeforeTest(LdaEngine engine)
   at Microsoft.ML.Transforms.Text.LatentDirichletAllocationTransformer.LdaState.Output(VBuffer`1& src, VBuffer`1& dst, Int32 numBurninIter, Boolean reset) in D:\a\1\s\src\Microsoft.ML.Transforms\Text\LdaTransform.cs:line 470
   at Microsoft.ML.Transforms.Text.LatentDirichletAllocationTransformer.Mapper.<>c__DisplayClass5_0.<GetTopic>b__0(VBuffer`1& dst) in D:\a\1\s\src\Microsoft.ML.Transforms\Text\LdaTransform.cs:line 612
   at Microsoft.ML.Data.DataViewUtils.Splitter.InPipe.Impl`1.Fill() in D:\a\1\s\src\Microsoft.ML.Data\Data\DataViewUtils.cs:line 723
   at Microsoft.ML.Data.DataViewUtils.Splitter.<>c__DisplayClass5_1.<ConsolidateCore>b__2() in D:\a\1\s\src\Microsoft.ML.Data\Data\DataViewUtils.cs:line 415

@sharwell sharwell merged commit 8221dac into dotnet:master Dec 9, 2019
@sharwell sharwell deleted the safe-lda-engine-handle branch December 9, 2019 15:30
@ghost ghost locked as resolved and limited conversation to collaborators Mar 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants