Fix marshalling of bool flags in MF by wschin · Pull Request #3210 · dotnet/machinelearning

wschin · 2019-04-05T03:17:15Z

Fix#3003. The marshalled Boolean size from C# to C should be 1 byte, not 4. Also, the change from LIBMF is for supporting quiet option in new implicit-feedback MF.

codecov · 2019-04-05T03:54:44Z

Codecov Report

Merging #3210 into master will increase coverage by 0.01%.
The diff coverage is 0%.

@@ Coverage Diff @@## master #3210 +/- ##
==========================================
+ Coverage 72.61% 72.62% +0.01% 
==========================================
Files 807 807 Lines 145077 145080 +3 Branches 16213 16213 ==========================================
+ Hits 105345 105365 +20 + Misses 35316 35297 -19 - Partials 4416 4418 +2

Flag	Coverage Δ
#Debug	`72.62% <0%> (+0.01%)`	⬆️
#production	`68.17% <0%> (+0.01%)`	⬆️
#test	`88.92% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
...osoft.ML.Recommender/SafeTrainingAndModelBuffer.cs	`78.87% <0%> (ø)`	⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	`84.7% <0%> (-0.41%)`	⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	`86.1% <0%> (-0.16%)`	⬇️
...Microsoft.ML.Transforms/FeatureSelectionCatalog.cs	`60% <0%> (ø)`	⬆️
...osoft.ML.Recommender/MatrixFactorizationTrainer.cs	`70.39% <0%> (ø)`	⬆️
.../Microsoft.ML.Tests/TrainerEstimators/SdcaTests.cs	`97.31% <0%> (+0.05%)`	⬆️
...oft.ML.StandardTrainers/Standard/SdcaMulticlass.cs	`91.12% <0%> (+1.02%)`	⬆️
...LogisticRegression/MulticlassLogisticRegression.cs	`67.61% <0%> (+1.74%)`	⬆️
...oft.ML.StandardTrainers/StandardTrainersCatalog.cs	`92.34% <0%> (+3.27%)`	⬆️

eerhardt · 2019-04-05T14:53:19Z

 /// </summary>
 [FieldOffset(48)]
- public int DoNmf;
+ public byte DoNmf;


While this is better than what we had before, this is still not correct.
https://docs.microsoft.com/en-us/cpp/cpp/fundamental-types-cpp?view=vs-2019
bool Type bool is an integral type that can have one of the two values true or false. Its size is unspecified.
https://en.cppreference.com/w/cpp/language/types
bool - type, capable of holding one of the two values: true or false. The value of sizeof(bool) is implementation defined and might differ from 1.
Instead, to make interop/marshalling work correctly on all computers - the technique we use in .NET is to ensure the ABI (application binary interface) is stable no matter the compiler/processor/etc. This means only stable-sized types can be used in the ABI that C# code P/Invokes into.
So to fix this, the libmf C code should change to have a stable ABI. I see it uses float, int, and bool. For sure int and bool can change sizes depending on the compiler/processor. Instead, we should be using int32_t and int8_t on the C side. (Note: you can just have a wrapper export function in the C library that exposes the stable ABI and calls into your current code. So the main C/C++ code can remain unchanged.) #Closed

Thank you for pointing this out. Another iteration is pushed. Hopefully, it can address this comment.

eerhardt · 2019-04-05T18:19:38Z

 /// </summary>
 [FieldOffset(48)]
- public int DoNmf;
+ public sbyte DoNmf;


Why sbyte? Why not just byte?

It's int8 in C side.

Make it uint8_t on the C side then?
Signed-ness doesn't really matter here. So using byte is more appropriate.

Ok. I will change other place to match this decision.

eerhardt · 2019-04-05T18:21:45Z


 using namespace mf;

+EXPORT_API(mf_parameter) make_param(const mf_parameter_bridge *param_bridge)


I don't think this needs EXPORT_API does it?

eerhardt · 2019-04-05T18:23:07Z

+ param.eta = param_bridge->eta;
+ param.alpha = param_bridge->alpha;
+ param.c = param_bridge->c;
+ param.do_nmf = static_cast<bool>(param_bridge->do_nmf);


Instead of static cast, would it be better to use param_bridge->do_nmf != 0 ? true : false?

eerhardt

wschin · 2019-04-05T22:05:30Z

I will make change to mf_problem and mf_model in other PRs because they incur more complicated changes.

wschin · 2019-04-05T03:17:15Z

Fix#3003. The marshalled Boolean size from C# to C should be 1 byte, not 4. Also, the change from LIBMF is for supporting quiet option in new implicit-feedback MF.

codecov · 2019-04-05T03:54:44Z

Codecov Report

Merging #3210 into master will increase coverage by 0.01%.
The diff coverage is 0%.

@@ Coverage Diff @@## master #3210 +/- ##
==========================================
+ Coverage 72.61% 72.62% +0.01% 
==========================================
Files 807 807 Lines 145077 145080 +3 Branches 16213 16213 ==========================================
+ Hits 105345 105365 +20 + Misses 35316 35297 -19 - Partials 4416 4418 +2

Flag	Coverage Δ
#Debug	`72.62% <0%> (+0.01%)`	⬆️
#production	`68.17% <0%> (+0.01%)`	⬆️
#test	`88.92% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
...osoft.ML.Recommender/SafeTrainingAndModelBuffer.cs	`78.87% <0%> (ø)`	⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	`84.7% <0%> (-0.41%)`	⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	`86.1% <0%> (-0.16%)`	⬇️
...Microsoft.ML.Transforms/FeatureSelectionCatalog.cs	`60% <0%> (ø)`	⬆️
...osoft.ML.Recommender/MatrixFactorizationTrainer.cs	`70.39% <0%> (ø)`	⬆️
.../Microsoft.ML.Tests/TrainerEstimators/SdcaTests.cs	`97.31% <0%> (+0.05%)`	⬆️
...oft.ML.StandardTrainers/Standard/SdcaMulticlass.cs	`91.12% <0%> (+1.02%)`	⬆️
...LogisticRegression/MulticlassLogisticRegression.cs	`67.61% <0%> (+1.74%)`	⬆️
...oft.ML.StandardTrainers/StandardTrainersCatalog.cs	`92.34% <0%> (+3.27%)`	⬆️

eerhardt · 2019-04-05T14:53:19Z

 /// </summary>
 [FieldOffset(48)]
- public int DoNmf;
+ public byte DoNmf;


While this is better than what we had before, this is still not correct.
https://docs.microsoft.com/en-us/cpp/cpp/fundamental-types-cpp?view=vs-2019
bool Type bool is an integral type that can have one of the two values true or false. Its size is unspecified.
https://en.cppreference.com/w/cpp/language/types
bool - type, capable of holding one of the two values: true or false. The value of sizeof(bool) is implementation defined and might differ from 1.
Instead, to make interop/marshalling work correctly on all computers - the technique we use in .NET is to ensure the ABI (application binary interface) is stable no matter the compiler/processor/etc. This means only stable-sized types can be used in the ABI that C# code P/Invokes into.
So to fix this, the libmf C code should change to have a stable ABI. I see it uses float, int, and bool. For sure int and bool can change sizes depending on the compiler/processor. Instead, we should be using int32_t and int8_t on the C side. (Note: you can just have a wrapper export function in the C library that exposes the stable ABI and calls into your current code. So the main C/C++ code can remain unchanged.) #Closed

Thank you for pointing this out. Another iteration is pushed. Hopefully, it can address this comment.

eerhardt · 2019-04-05T18:19:38Z

 /// </summary>
 [FieldOffset(48)]
- public int DoNmf;
+ public sbyte DoNmf;


Why sbyte? Why not just byte?

It's int8 in C side.

Make it uint8_t on the C side then?
Signed-ness doesn't really matter here. So using byte is more appropriate.

Ok. I will change other place to match this decision.

eerhardt · 2019-04-05T18:21:45Z


 using namespace mf;

+EXPORT_API(mf_parameter) make_param(const mf_parameter_bridge *param_bridge)


I don't think this needs EXPORT_API does it?

eerhardt · 2019-04-05T18:23:07Z

+ param.eta = param_bridge->eta;
+ param.alpha = param_bridge->alpha;
+ param.c = param_bridge->c;
+ param.do_nmf = static_cast<bool>(param_bridge->do_nmf);


Instead of static cast, would it be better to use param_bridge->do_nmf != 0 ? true : false?

eerhardt

wschin · 2019-04-05T22:05:30Z

I will make change to mf_problem and mf_model in other PRs because they incur more complicated changes.

wschin · 2019-04-05T03:17:15Z

Fix#3003. The marshalled Boolean size from C# to C should be 1 byte, not 4. Also, the change from LIBMF is for supporting quiet option in new implicit-feedback MF.

codecov · 2019-04-05T03:54:44Z

Codecov Report

Merging #3210 into master will increase coverage by 0.01%.
The diff coverage is 0%.

@@ Coverage Diff @@## master #3210 +/- ##
==========================================
+ Coverage 72.61% 72.62% +0.01% 
==========================================
Files 807 807 Lines 145077 145080 +3 Branches 16213 16213 ==========================================
+ Hits 105345 105365 +20 + Misses 35316 35297 -19 - Partials 4416 4418 +2

Flag	Coverage Δ
#Debug	`72.62% <0%> (+0.01%)`	⬆️
#production	`68.17% <0%> (+0.01%)`	⬆️
#test	`88.92% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
...osoft.ML.Recommender/SafeTrainingAndModelBuffer.cs	`78.87% <0%> (ø)`	⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	`84.7% <0%> (-0.41%)`	⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	`86.1% <0%> (-0.16%)`	⬇️
...Microsoft.ML.Transforms/FeatureSelectionCatalog.cs	`60% <0%> (ø)`	⬆️
...osoft.ML.Recommender/MatrixFactorizationTrainer.cs	`70.39% <0%> (ø)`	⬆️
.../Microsoft.ML.Tests/TrainerEstimators/SdcaTests.cs	`97.31% <0%> (+0.05%)`	⬆️
...oft.ML.StandardTrainers/Standard/SdcaMulticlass.cs	`91.12% <0%> (+1.02%)`	⬆️
...LogisticRegression/MulticlassLogisticRegression.cs	`67.61% <0%> (+1.74%)`	⬆️
...oft.ML.StandardTrainers/StandardTrainersCatalog.cs	`92.34% <0%> (+3.27%)`	⬆️

eerhardt · 2019-04-05T14:53:19Z

 /// </summary>
 [FieldOffset(48)]
- public int DoNmf;
+ public byte DoNmf;


While this is better than what we had before, this is still not correct.
https://docs.microsoft.com/en-us/cpp/cpp/fundamental-types-cpp?view=vs-2019
bool Type bool is an integral type that can have one of the two values true or false. Its size is unspecified.
https://en.cppreference.com/w/cpp/language/types
bool - type, capable of holding one of the two values: true or false. The value of sizeof(bool) is implementation defined and might differ from 1.
Instead, to make interop/marshalling work correctly on all computers - the technique we use in .NET is to ensure the ABI (application binary interface) is stable no matter the compiler/processor/etc. This means only stable-sized types can be used in the ABI that C# code P/Invokes into.
So to fix this, the libmf C code should change to have a stable ABI. I see it uses float, int, and bool. For sure int and bool can change sizes depending on the compiler/processor. Instead, we should be using int32_t and int8_t on the C side. (Note: you can just have a wrapper export function in the C library that exposes the stable ABI and calls into your current code. So the main C/C++ code can remain unchanged.) #Closed

Thank you for pointing this out. Another iteration is pushed. Hopefully, it can address this comment.

eerhardt · 2019-04-05T18:19:38Z

 /// </summary>
 [FieldOffset(48)]
- public int DoNmf;
+ public sbyte DoNmf;


Why sbyte? Why not just byte?

It's int8 in C side.

Make it uint8_t on the C side then?
Signed-ness doesn't really matter here. So using byte is more appropriate.

Ok. I will change other place to match this decision.

eerhardt · 2019-04-05T18:21:45Z


 using namespace mf;

+EXPORT_API(mf_parameter) make_param(const mf_parameter_bridge *param_bridge)


I don't think this needs EXPORT_API does it?

eerhardt · 2019-04-05T18:23:07Z

+ param.eta = param_bridge->eta;
+ param.alpha = param_bridge->alpha;
+ param.c = param_bridge->c;
+ param.do_nmf = static_cast<bool>(param_bridge->do_nmf);


Instead of static cast, would it be better to use param_bridge->do_nmf != 0 ? true : false?

eerhardt

wschin · 2019-04-05T22:05:30Z

I will make change to mf_problem and mf_model in other PRs because they incur more complicated changes.

wschin · 2019-04-05T03:17:15Z

Fix#3003. The marshalled Boolean size from C# to C should be 1 byte, not 4. Also, the change from LIBMF is for supporting quiet option in new implicit-feedback MF.

codecov · 2019-04-05T03:54:44Z

Codecov Report

Merging #3210 into master will increase coverage by 0.01%.
The diff coverage is 0%.

@@ Coverage Diff @@## master #3210 +/- ##
==========================================
+ Coverage 72.61% 72.62% +0.01% 
==========================================
Files 807 807 Lines 145077 145080 +3 Branches 16213 16213 ==========================================
+ Hits 105345 105365 +20 + Misses 35316 35297 -19 - Partials 4416 4418 +2

Flag	Coverage Δ
#Debug	`72.62% <0%> (+0.01%)`	⬆️
#production	`68.17% <0%> (+0.01%)`	⬆️
#test	`88.92% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
...osoft.ML.Recommender/SafeTrainingAndModelBuffer.cs	`78.87% <0%> (ø)`	⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	`84.7% <0%> (-0.41%)`	⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	`86.1% <0%> (-0.16%)`	⬇️
...Microsoft.ML.Transforms/FeatureSelectionCatalog.cs	`60% <0%> (ø)`	⬆️
...osoft.ML.Recommender/MatrixFactorizationTrainer.cs	`70.39% <0%> (ø)`	⬆️
.../Microsoft.ML.Tests/TrainerEstimators/SdcaTests.cs	`97.31% <0%> (+0.05%)`	⬆️
...oft.ML.StandardTrainers/Standard/SdcaMulticlass.cs	`91.12% <0%> (+1.02%)`	⬆️
...LogisticRegression/MulticlassLogisticRegression.cs	`67.61% <0%> (+1.74%)`	⬆️
...oft.ML.StandardTrainers/StandardTrainersCatalog.cs	`92.34% <0%> (+3.27%)`	⬆️

eerhardt · 2019-04-05T14:53:19Z

 /// </summary>
 [FieldOffset(48)]
- public int DoNmf;
+ public byte DoNmf;


While this is better than what we had before, this is still not correct.
https://docs.microsoft.com/en-us/cpp/cpp/fundamental-types-cpp?view=vs-2019
bool Type bool is an integral type that can have one of the two values true or false. Its size is unspecified.
https://en.cppreference.com/w/cpp/language/types
bool - type, capable of holding one of the two values: true or false. The value of sizeof(bool) is implementation defined and might differ from 1.
Instead, to make interop/marshalling work correctly on all computers - the technique we use in .NET is to ensure the ABI (application binary interface) is stable no matter the compiler/processor/etc. This means only stable-sized types can be used in the ABI that C# code P/Invokes into.
So to fix this, the libmf C code should change to have a stable ABI. I see it uses float, int, and bool. For sure int and bool can change sizes depending on the compiler/processor. Instead, we should be using int32_t and int8_t on the C side. (Note: you can just have a wrapper export function in the C library that exposes the stable ABI and calls into your current code. So the main C/C++ code can remain unchanged.) #Closed

Thank you for pointing this out. Another iteration is pushed. Hopefully, it can address this comment.

eerhardt · 2019-04-05T18:19:38Z

 /// </summary>
 [FieldOffset(48)]
- public int DoNmf;
+ public sbyte DoNmf;


Why sbyte? Why not just byte?

It's int8 in C side.

Make it uint8_t on the C side then?
Signed-ness doesn't really matter here. So using byte is more appropriate.

Ok. I will change other place to match this decision.

eerhardt · 2019-04-05T18:21:45Z


 using namespace mf;

+EXPORT_API(mf_parameter) make_param(const mf_parameter_bridge *param_bridge)


I don't think this needs EXPORT_API does it?

eerhardt · 2019-04-05T18:23:07Z

+ param.eta = param_bridge->eta;
+ param.alpha = param_bridge->alpha;
+ param.c = param_bridge->c;
+ param.do_nmf = static_cast<bool>(param_bridge->do_nmf);


Instead of static cast, would it be better to use param_bridge->do_nmf != 0 ? true : false?

eerhardt

wschin · 2019-04-05T22:05:30Z

I will make change to mf_problem and mf_model in other PRs because they incur more complicated changes.

wschin · 2019-04-05T03:17:15Z

Fix#3003. The marshalled Boolean size from C# to C should be 1 byte, not 4. Also, the change from LIBMF is for supporting quiet option in new implicit-feedback MF.

codecov · 2019-04-05T03:54:44Z

Codecov Report

Merging #3210 into master will increase coverage by 0.01%.
The diff coverage is 0%.

@@ Coverage Diff @@## master #3210 +/- ##
==========================================
+ Coverage 72.61% 72.62% +0.01% 
==========================================
Files 807 807 Lines 145077 145080 +3 Branches 16213 16213 ==========================================
+ Hits 105345 105365 +20 + Misses 35316 35297 -19 - Partials 4416 4418 +2

Flag	Coverage Δ
#Debug	`72.62% <0%> (+0.01%)`	⬆️
#production	`68.17% <0%> (+0.01%)`	⬆️
#test	`88.92% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
...osoft.ML.Recommender/SafeTrainingAndModelBuffer.cs	`78.87% <0%> (ø)`	⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	`84.7% <0%> (-0.41%)`	⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	`86.1% <0%> (-0.16%)`	⬇️
...Microsoft.ML.Transforms/FeatureSelectionCatalog.cs	`60% <0%> (ø)`	⬆️
...osoft.ML.Recommender/MatrixFactorizationTrainer.cs	`70.39% <0%> (ø)`	⬆️
.../Microsoft.ML.Tests/TrainerEstimators/SdcaTests.cs	`97.31% <0%> (+0.05%)`	⬆️
...oft.ML.StandardTrainers/Standard/SdcaMulticlass.cs	`91.12% <0%> (+1.02%)`	⬆️
...LogisticRegression/MulticlassLogisticRegression.cs	`67.61% <0%> (+1.74%)`	⬆️
...oft.ML.StandardTrainers/StandardTrainersCatalog.cs	`92.34% <0%> (+3.27%)`	⬆️

eerhardt · 2019-04-05T14:53:19Z

 /// </summary>
 [FieldOffset(48)]
- public int DoNmf;
+ public byte DoNmf;


While this is better than what we had before, this is still not correct.
https://docs.microsoft.com/en-us/cpp/cpp/fundamental-types-cpp?view=vs-2019
bool Type bool is an integral type that can have one of the two values true or false. Its size is unspecified.
https://en.cppreference.com/w/cpp/language/types
bool - type, capable of holding one of the two values: true or false. The value of sizeof(bool) is implementation defined and might differ from 1.
Instead, to make interop/marshalling work correctly on all computers - the technique we use in .NET is to ensure the ABI (application binary interface) is stable no matter the compiler/processor/etc. This means only stable-sized types can be used in the ABI that C# code P/Invokes into.
So to fix this, the libmf C code should change to have a stable ABI. I see it uses float, int, and bool. For sure int and bool can change sizes depending on the compiler/processor. Instead, we should be using int32_t and int8_t on the C side. (Note: you can just have a wrapper export function in the C library that exposes the stable ABI and calls into your current code. So the main C/C++ code can remain unchanged.) #Closed

Thank you for pointing this out. Another iteration is pushed. Hopefully, it can address this comment.

eerhardt · 2019-04-05T18:19:38Z

 /// </summary>
 [FieldOffset(48)]
- public int DoNmf;
+ public sbyte DoNmf;


Why sbyte? Why not just byte?

It's int8 in C side.

Make it uint8_t on the C side then?
Signed-ness doesn't really matter here. So using byte is more appropriate.

Ok. I will change other place to match this decision.

eerhardt · 2019-04-05T18:21:45Z


 using namespace mf;

+EXPORT_API(mf_parameter) make_param(const mf_parameter_bridge *param_bridge)


I don't think this needs EXPORT_API does it?

eerhardt · 2019-04-05T18:23:07Z

+ param.eta = param_bridge->eta;
+ param.alpha = param_bridge->alpha;
+ param.c = param_bridge->c;
+ param.do_nmf = static_cast<bool>(param_bridge->do_nmf);


Instead of static cast, would it be better to use param_bridge->do_nmf != 0 ? true : false?

eerhardt

wschin · 2019-04-05T22:05:30Z

I will make change to mf_problem and mf_model in other PRs because they incur more complicated changes.

wschin · 2019-04-05T03:17:15Z

Fix#3003. The marshalled Boolean size from C# to C should be 1 byte, not 4. Also, the change from LIBMF is for supporting quiet option in new implicit-feedback MF.

codecov · 2019-04-05T03:54:44Z

Codecov Report

Merging #3210 into master will increase coverage by 0.01%.
The diff coverage is 0%.

@@ Coverage Diff @@## master #3210 +/- ##
==========================================
+ Coverage 72.61% 72.62% +0.01% 
==========================================
Files 807 807 Lines 145077 145080 +3 Branches 16213 16213 ==========================================
+ Hits 105345 105365 +20 + Misses 35316 35297 -19 - Partials 4416 4418 +2

Flag	Coverage Δ
#Debug	`72.62% <0%> (+0.01%)`	⬆️
#production	`68.17% <0%> (+0.01%)`	⬆️
#test	`88.92% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
...osoft.ML.Recommender/SafeTrainingAndModelBuffer.cs	`78.87% <0%> (ø)`	⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	`84.7% <0%> (-0.41%)`	⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	`86.1% <0%> (-0.16%)`	⬇️
...Microsoft.ML.Transforms/FeatureSelectionCatalog.cs	`60% <0%> (ø)`	⬆️
...osoft.ML.Recommender/MatrixFactorizationTrainer.cs	`70.39% <0%> (ø)`	⬆️
.../Microsoft.ML.Tests/TrainerEstimators/SdcaTests.cs	`97.31% <0%> (+0.05%)`	⬆️
...oft.ML.StandardTrainers/Standard/SdcaMulticlass.cs	`91.12% <0%> (+1.02%)`	⬆️
...LogisticRegression/MulticlassLogisticRegression.cs	`67.61% <0%> (+1.74%)`	⬆️
...oft.ML.StandardTrainers/StandardTrainersCatalog.cs	`92.34% <0%> (+3.27%)`	⬆️

eerhardt · 2019-04-05T14:53:19Z

 /// </summary>
 [FieldOffset(48)]
- public int DoNmf;
+ public byte DoNmf;


While this is better than what we had before, this is still not correct.
https://docs.microsoft.com/en-us/cpp/cpp/fundamental-types-cpp?view=vs-2019
bool Type bool is an integral type that can have one of the two values true or false. Its size is unspecified.
https://en.cppreference.com/w/cpp/language/types
bool - type, capable of holding one of the two values: true or false. The value of sizeof(bool) is implementation defined and might differ from 1.
Instead, to make interop/marshalling work correctly on all computers - the technique we use in .NET is to ensure the ABI (application binary interface) is stable no matter the compiler/processor/etc. This means only stable-sized types can be used in the ABI that C# code P/Invokes into.
So to fix this, the libmf C code should change to have a stable ABI. I see it uses float, int, and bool. For sure int and bool can change sizes depending on the compiler/processor. Instead, we should be using int32_t and int8_t on the C side. (Note: you can just have a wrapper export function in the C library that exposes the stable ABI and calls into your current code. So the main C/C++ code can remain unchanged.) #Closed

Thank you for pointing this out. Another iteration is pushed. Hopefully, it can address this comment.

eerhardt · 2019-04-05T18:19:38Z

 /// </summary>
 [FieldOffset(48)]
- public int DoNmf;
+ public sbyte DoNmf;


Why sbyte? Why not just byte?

It's int8 in C side.

Make it uint8_t on the C side then?
Signed-ness doesn't really matter here. So using byte is more appropriate.

Ok. I will change other place to match this decision.

eerhardt · 2019-04-05T18:21:45Z


 using namespace mf;

+EXPORT_API(mf_parameter) make_param(const mf_parameter_bridge *param_bridge)


I don't think this needs EXPORT_API does it?

eerhardt · 2019-04-05T18:23:07Z

+ param.eta = param_bridge->eta;
+ param.alpha = param_bridge->alpha;
+ param.c = param_bridge->c;
+ param.do_nmf = static_cast<bool>(param_bridge->do_nmf);


Instead of static cast, would it be better to use param_bridge->do_nmf != 0 ? true : false?

eerhardt

wschin · 2019-04-05T22:05:30Z

I will make change to mf_problem and mf_model in other PRs because they incur more complicated changes.

wschin · 2019-04-05T03:17:15Z

Fix#3003. The marshalled Boolean size from C# to C should be 1 byte, not 4. Also, the change from LIBMF is for supporting quiet option in new implicit-feedback MF.

codecov · 2019-04-05T03:54:44Z

Codecov Report

Merging #3210 into master will increase coverage by 0.01%.
The diff coverage is 0%.

@@ Coverage Diff @@## master #3210 +/- ##
==========================================
+ Coverage 72.61% 72.62% +0.01% 
==========================================
Files 807 807 Lines 145077 145080 +3 Branches 16213 16213 ==========================================
+ Hits 105345 105365 +20 + Misses 35316 35297 -19 - Partials 4416 4418 +2

Flag	Coverage Δ
#Debug	`72.62% <0%> (+0.01%)`	⬆️
#production	`68.17% <0%> (+0.01%)`	⬆️
#test	`88.92% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
...osoft.ML.Recommender/SafeTrainingAndModelBuffer.cs	`78.87% <0%> (ø)`	⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	`84.7% <0%> (-0.41%)`	⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	`86.1% <0%> (-0.16%)`	⬇️
...Microsoft.ML.Transforms/FeatureSelectionCatalog.cs	`60% <0%> (ø)`	⬆️
...osoft.ML.Recommender/MatrixFactorizationTrainer.cs	`70.39% <0%> (ø)`	⬆️
.../Microsoft.ML.Tests/TrainerEstimators/SdcaTests.cs	`97.31% <0%> (+0.05%)`	⬆️
...oft.ML.StandardTrainers/Standard/SdcaMulticlass.cs	`91.12% <0%> (+1.02%)`	⬆️
...LogisticRegression/MulticlassLogisticRegression.cs	`67.61% <0%> (+1.74%)`	⬆️
...oft.ML.StandardTrainers/StandardTrainersCatalog.cs	`92.34% <0%> (+3.27%)`	⬆️

eerhardt · 2019-04-05T14:53:19Z

 /// </summary>
 [FieldOffset(48)]
- public int DoNmf;
+ public byte DoNmf;


While this is better than what we had before, this is still not correct.
https://docs.microsoft.com/en-us/cpp/cpp/fundamental-types-cpp?view=vs-2019
bool Type bool is an integral type that can have one of the two values true or false. Its size is unspecified.
https://en.cppreference.com/w/cpp/language/types
bool - type, capable of holding one of the two values: true or false. The value of sizeof(bool) is implementation defined and might differ from 1.
Instead, to make interop/marshalling work correctly on all computers - the technique we use in .NET is to ensure the ABI (application binary interface) is stable no matter the compiler/processor/etc. This means only stable-sized types can be used in the ABI that C# code P/Invokes into.
So to fix this, the libmf C code should change to have a stable ABI. I see it uses float, int, and bool. For sure int and bool can change sizes depending on the compiler/processor. Instead, we should be using int32_t and int8_t on the C side. (Note: you can just have a wrapper export function in the C library that exposes the stable ABI and calls into your current code. So the main C/C++ code can remain unchanged.) #Closed

Thank you for pointing this out. Another iteration is pushed. Hopefully, it can address this comment.

eerhardt · 2019-04-05T18:19:38Z

 /// </summary>
 [FieldOffset(48)]
- public int DoNmf;
+ public sbyte DoNmf;


Why sbyte? Why not just byte?

It's int8 in C side.

Make it uint8_t on the C side then?
Signed-ness doesn't really matter here. So using byte is more appropriate.

Ok. I will change other place to match this decision.

eerhardt · 2019-04-05T18:21:45Z


 using namespace mf;

+EXPORT_API(mf_parameter) make_param(const mf_parameter_bridge *param_bridge)


I don't think this needs EXPORT_API does it?

eerhardt · 2019-04-05T18:23:07Z

+ param.eta = param_bridge->eta;
+ param.alpha = param_bridge->alpha;
+ param.c = param_bridge->c;
+ param.do_nmf = static_cast<bool>(param_bridge->do_nmf);


Instead of static cast, would it be better to use param_bridge->do_nmf != 0 ? true : false?

eerhardt

wschin · 2019-04-05T22:05:30Z

I will make change to mf_problem and mf_model in other PRs because they incur more complicated changes.

wschin · 2019-04-05T03:17:15Z

Fix#3003. The marshalled Boolean size from C# to C should be 1 byte, not 4. Also, the change from LIBMF is for supporting quiet option in new implicit-feedback MF.

codecov · 2019-04-05T03:54:44Z

Codecov Report

Merging #3210 into master will increase coverage by 0.01%.
The diff coverage is 0%.

@@ Coverage Diff @@## master #3210 +/- ##
==========================================
+ Coverage 72.61% 72.62% +0.01% 
==========================================
Files 807 807 Lines 145077 145080 +3 Branches 16213 16213 ==========================================
+ Hits 105345 105365 +20 + Misses 35316 35297 -19 - Partials 4416 4418 +2

Flag	Coverage Δ
#Debug	`72.62% <0%> (+0.01%)`	⬆️
#production	`68.17% <0%> (+0.01%)`	⬆️
#test	`88.92% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
...osoft.ML.Recommender/SafeTrainingAndModelBuffer.cs	`78.87% <0%> (ø)`	⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	`84.7% <0%> (-0.41%)`	⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	`86.1% <0%> (-0.16%)`	⬇️
...Microsoft.ML.Transforms/FeatureSelectionCatalog.cs	`60% <0%> (ø)`	⬆️
...osoft.ML.Recommender/MatrixFactorizationTrainer.cs	`70.39% <0%> (ø)`	⬆️
.../Microsoft.ML.Tests/TrainerEstimators/SdcaTests.cs	`97.31% <0%> (+0.05%)`	⬆️
...oft.ML.StandardTrainers/Standard/SdcaMulticlass.cs	`91.12% <0%> (+1.02%)`	⬆️
...LogisticRegression/MulticlassLogisticRegression.cs	`67.61% <0%> (+1.74%)`	⬆️
...oft.ML.StandardTrainers/StandardTrainersCatalog.cs	`92.34% <0%> (+3.27%)`	⬆️

eerhardt · 2019-04-05T14:53:19Z

 /// </summary>
 [FieldOffset(48)]
- public int DoNmf;
+ public byte DoNmf;


While this is better than what we had before, this is still not correct.
https://docs.microsoft.com/en-us/cpp/cpp/fundamental-types-cpp?view=vs-2019
bool Type bool is an integral type that can have one of the two values true or false. Its size is unspecified.
https://en.cppreference.com/w/cpp/language/types
bool - type, capable of holding one of the two values: true or false. The value of sizeof(bool) is implementation defined and might differ from 1.
Instead, to make interop/marshalling work correctly on all computers - the technique we use in .NET is to ensure the ABI (application binary interface) is stable no matter the compiler/processor/etc. This means only stable-sized types can be used in the ABI that C# code P/Invokes into.
So to fix this, the libmf C code should change to have a stable ABI. I see it uses float, int, and bool. For sure int and bool can change sizes depending on the compiler/processor. Instead, we should be using int32_t and int8_t on the C side. (Note: you can just have a wrapper export function in the C library that exposes the stable ABI and calls into your current code. So the main C/C++ code can remain unchanged.) #Closed

Thank you for pointing this out. Another iteration is pushed. Hopefully, it can address this comment.

eerhardt · 2019-04-05T18:19:38Z

 /// </summary>
 [FieldOffset(48)]
- public int DoNmf;
+ public sbyte DoNmf;


Why sbyte? Why not just byte?

It's int8 in C side.

Make it uint8_t on the C side then?
Signed-ness doesn't really matter here. So using byte is more appropriate.

Ok. I will change other place to match this decision.

eerhardt · 2019-04-05T18:21:45Z


 using namespace mf;

+EXPORT_API(mf_parameter) make_param(const mf_parameter_bridge *param_bridge)


I don't think this needs EXPORT_API does it?

eerhardt · 2019-04-05T18:23:07Z

+ param.eta = param_bridge->eta;
+ param.alpha = param_bridge->alpha;
+ param.c = param_bridge->c;
+ param.do_nmf = static_cast<bool>(param_bridge->do_nmf);


Instead of static cast, would it be better to use param_bridge->do_nmf != 0 ? true : false?

eerhardt

wschin · 2019-04-05T22:05:30Z

I will make change to mf_problem and mf_model in other PRs because they incur more complicated changes.

Fix marshalling of bool flags in MF
2c8090c

wschin requested review from Ivanidzo4ka and eerhardt April 5, 2019 03:17

wschin self-assigned this Apr 5, 2019

wschin added the bug Something isn't working label Apr 5, 2019

Ivanidzo4ka approved these changes Apr 5, 2019
View reviewed changes

eerhardt reviewed Apr 5, 2019
View reviewed changes

Address comment
fbbc121

wschin requested a review from eerhardt April 5, 2019 18:16

eerhardt reviewed Apr 5, 2019
View reviewed changes

eerhardt approved these changes Apr 5, 2019
View reviewed changes

Better type of C# and C bool bridge
1bcdc54

wschin merged commit b2ac8e0 into dotnet:masterApr 5, 2019

wschin deleted the quiet-mf branch April 5, 2019 22:05

wschin mentioned this pull request Apr 5, 2019
Polish marshalling of MF model and MF problem and enable 32-bit tests #3227
Merged

wschin added a commit to wschin/machinelearning that referenced this pull request Apr 11, 2019
Fix marshalling of bool flags in MF (dotnet#3210)
7d842d8

wschin mentioned this pull request Apr 11, 2019
Add MF fixes to release branch #3292
Closed

ghost locked as resolved and limited conversation to collaborators Mar 23, 2022

Fix marshalling of bool flags in MF
2c8090c

wschin requested review from Ivanidzo4ka and eerhardt April 5, 2019 03:17

wschin self-assigned this Apr 5, 2019

wschin added the bug Something isn't working label Apr 5, 2019

Ivanidzo4ka approved these changes Apr 5, 2019
View reviewed changes

eerhardt reviewed Apr 5, 2019
View reviewed changes

Address comment
fbbc121

wschin requested a review from eerhardt April 5, 2019 18:16

eerhardt reviewed Apr 5, 2019
View reviewed changes

eerhardt approved these changes Apr 5, 2019
View reviewed changes

Better type of C# and C bool bridge
1bcdc54

wschin merged commit b2ac8e0 into dotnet:masterApr 5, 2019

wschin deleted the quiet-mf branch April 5, 2019 22:05

wschin mentioned this pull request Apr 5, 2019
Polish marshalling of MF model and MF problem and enable 32-bit tests #3227
Merged

wschin added a commit to wschin/machinelearning that referenced this pull request Apr 11, 2019
Fix marshalling of bool flags in MF (dotnet#3210)
7d842d8

wschin mentioned this pull request Apr 11, 2019
Add MF fixes to release branch #3292
Closed

ghost locked as resolved and limited conversation to collaborators Mar 23, 2022

Fix marshalling of bool flags in MF
2c8090c

wschin requested review from Ivanidzo4ka and eerhardt April 5, 2019 03:17

wschin self-assigned this Apr 5, 2019

wschin added the bug Something isn't working label Apr 5, 2019

Ivanidzo4ka approved these changes Apr 5, 2019
View reviewed changes

eerhardt reviewed Apr 5, 2019
View reviewed changes

Address comment
fbbc121

wschin requested a review from eerhardt April 5, 2019 18:16

eerhardt reviewed Apr 5, 2019
View reviewed changes

eerhardt approved these changes Apr 5, 2019
View reviewed changes

Better type of C# and C bool bridge
1bcdc54

wschin merged commit b2ac8e0 into dotnet:masterApr 5, 2019

wschin deleted the quiet-mf branch April 5, 2019 22:05

wschin mentioned this pull request Apr 5, 2019
Polish marshalling of MF model and MF problem and enable 32-bit tests #3227
Merged

wschin added a commit to wschin/machinelearning that referenced this pull request Apr 11, 2019
Fix marshalling of bool flags in MF (dotnet#3210)
7d842d8

wschin mentioned this pull request Apr 11, 2019
Add MF fixes to release branch #3292
Closed

ghost locked as resolved and limited conversation to collaborators Mar 23, 2022

Fix marshalling of bool flags in MF
2c8090c

wschin requested review from Ivanidzo4ka and eerhardt April 5, 2019 03:17

wschin self-assigned this Apr 5, 2019

wschin added the bug Something isn't working label Apr 5, 2019

Ivanidzo4ka approved these changes Apr 5, 2019
View reviewed changes

eerhardt reviewed Apr 5, 2019
View reviewed changes

Address comment
fbbc121

wschin requested a review from eerhardt April 5, 2019 18:16

eerhardt reviewed Apr 5, 2019
View reviewed changes

eerhardt approved these changes Apr 5, 2019
View reviewed changes

Better type of C# and C bool bridge
1bcdc54

wschin merged commit b2ac8e0 into dotnet:masterApr 5, 2019

wschin deleted the quiet-mf branch April 5, 2019 22:05

wschin mentioned this pull request Apr 5, 2019
Polish marshalling of MF model and MF problem and enable 32-bit tests #3227
Merged

wschin added a commit to wschin/machinelearning that referenced this pull request Apr 11, 2019
Fix marshalling of bool flags in MF (dotnet#3210)
7d842d8

wschin mentioned this pull request Apr 11, 2019
Add MF fixes to release branch #3292
Closed

ghost locked as resolved and limited conversation to collaborators Mar 23, 2022

Fix marshalling of bool flags in MF
2c8090c

wschin requested review from Ivanidzo4ka and eerhardt April 5, 2019 03:17

wschin self-assigned this Apr 5, 2019

wschin added the bug Something isn't working label Apr 5, 2019

Ivanidzo4ka approved these changes Apr 5, 2019
View reviewed changes

eerhardt reviewed Apr 5, 2019
View reviewed changes

Address comment
fbbc121

wschin requested a review from eerhardt April 5, 2019 18:16

eerhardt reviewed Apr 5, 2019
View reviewed changes

eerhardt approved these changes Apr 5, 2019
View reviewed changes

Better type of C# and C bool bridge
1bcdc54

wschin merged commit b2ac8e0 into dotnet:masterApr 5, 2019

wschin deleted the quiet-mf branch April 5, 2019 22:05

wschin mentioned this pull request Apr 5, 2019
Polish marshalling of MF model and MF problem and enable 32-bit tests #3227
Merged

wschin added a commit to wschin/machinelearning that referenced this pull request Apr 11, 2019
Fix marshalling of bool flags in MF (dotnet#3210)
7d842d8

wschin mentioned this pull request Apr 11, 2019
Add MF fixes to release branch #3292
Closed

ghost locked as resolved and limited conversation to collaborators Mar 23, 2022

Fix marshalling of bool flags in MF
2c8090c

wschin requested review from Ivanidzo4ka and eerhardt April 5, 2019 03:17

wschin self-assigned this Apr 5, 2019

wschin added the bug Something isn't working label Apr 5, 2019

Ivanidzo4ka approved these changes Apr 5, 2019
View reviewed changes

eerhardt reviewed Apr 5, 2019
View reviewed changes

Address comment
fbbc121

wschin requested a review from eerhardt April 5, 2019 18:16

eerhardt reviewed Apr 5, 2019
View reviewed changes

eerhardt approved these changes Apr 5, 2019
View reviewed changes

Better type of C# and C bool bridge
1bcdc54

wschin merged commit b2ac8e0 into dotnet:masterApr 5, 2019

wschin deleted the quiet-mf branch April 5, 2019 22:05

wschin mentioned this pull request Apr 5, 2019
Polish marshalling of MF model and MF problem and enable 32-bit tests #3227
Merged

wschin added a commit to wschin/machinelearning that referenced this pull request Apr 11, 2019
Fix marshalling of bool flags in MF (dotnet#3210)
7d842d8

wschin mentioned this pull request Apr 11, 2019
Add MF fixes to release branch #3292
Closed

ghost locked as resolved and limited conversation to collaborators Mar 23, 2022

Fix marshalling of bool flags in MF
2c8090c

wschin requested review from Ivanidzo4ka and eerhardt April 5, 2019 03:17

wschin self-assigned this Apr 5, 2019

wschin added the bug Something isn't working label Apr 5, 2019

Ivanidzo4ka approved these changes Apr 5, 2019
View reviewed changes

eerhardt reviewed Apr 5, 2019
View reviewed changes

Address comment
fbbc121

wschin requested a review from eerhardt April 5, 2019 18:16

eerhardt reviewed Apr 5, 2019
View reviewed changes

eerhardt approved these changes Apr 5, 2019
View reviewed changes

Better type of C# and C bool bridge
1bcdc54

wschin merged commit b2ac8e0 into dotnet:masterApr 5, 2019

wschin deleted the quiet-mf branch April 5, 2019 22:05

wschin mentioned this pull request Apr 5, 2019
Polish marshalling of MF model and MF problem and enable 32-bit tests #3227
Merged

wschin added a commit to wschin/machinelearning that referenced this pull request Apr 11, 2019
Fix marshalling of bool flags in MF (dotnet#3210)
7d842d8

wschin mentioned this pull request Apr 11, 2019
Add MF fixes to release branch #3292
Closed

ghost locked as resolved and limited conversation to collaborators Mar 23, 2022

Fix marshalling of bool flags in MF
2c8090c

wschin requested review from Ivanidzo4ka and eerhardt April 5, 2019 03:17

wschin self-assigned this Apr 5, 2019

wschin added the bug Something isn't working label Apr 5, 2019

Ivanidzo4ka approved these changes Apr 5, 2019
View reviewed changes

eerhardt reviewed Apr 5, 2019
View reviewed changes

Address comment
fbbc121

wschin requested a review from eerhardt April 5, 2019 18:16

eerhardt reviewed Apr 5, 2019
View reviewed changes

eerhardt approved these changes Apr 5, 2019
View reviewed changes

Better type of C# and C bool bridge
1bcdc54

wschin merged commit b2ac8e0 into dotnet:masterApr 5, 2019

wschin deleted the quiet-mf branch April 5, 2019 22:05

wschin mentioned this pull request Apr 5, 2019
Polish marshalling of MF model and MF problem and enable 32-bit tests #3227
Merged

wschin added a commit to wschin/machinelearning that referenced this pull request Apr 11, 2019
Fix marshalling of bool flags in MF (dotnet#3210)
7d842d8

wschin mentioned this pull request Apr 11, 2019
Add MF fixes to release branch #3292
Closed

ghost locked as resolved and limited conversation to collaborators Mar 23, 2022


		using namespace mf;

		EXPORT_API(mf_parameter) make_param(const mf_parameter_bridge *param_bridge)

Uh oh!

Conversation

wschin commented Apr 5, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecovBot commented Apr 5, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

eerhardtApr 5, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wschinApr 5, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

wschin commented Apr 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

wschin commented Apr 5, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecovBot commented Apr 5, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

eerhardtApr 5, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wschinApr 5, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt left a comment

wschin commented Apr 5, 2019•
edited
Loading

codecovBot commented Apr 5, 2019•
edited
Loading

eerhardtApr 5, 2019•
edited
Loading

wschinApr 5, 2019•
edited
Loading

wschin commented Apr 5, 2019•
edited
Loading

codecovBot commented Apr 5, 2019•
edited
Loading

eerhardtApr 5, 2019•
edited
Loading

wschinApr 5, 2019•
edited
Loading

wschin commented Apr 5, 2019•
edited
Loading

codecovBot commented Apr 5, 2019•
edited
Loading

eerhardtApr 5, 2019•
edited
Loading