Added fix for test_msgpack_float and test_msgpack_object #2155

tvukovic-amd · 2023-09-06T09:23:35Z

fixed result comparison in msgpack test with float and map

migraphx-bot · 2023-09-06T14:19:19Z

Test	Batch	Rate new d2fd5a	Rate old 65c37c	Diff	Compare
torchvision-resnet50	64	2,323.80	2,324.63	-0.04%	✅
torchvision-resnet50_fp16	64	5,353.41	5,357.98	-0.09%	✅
torchvision-densenet121	32	1,846.94	1,847.88	-0.05%	✅
torchvision-densenet121_fp16	32	3,413.76	3,409.96	0.11%	✅
torchvision-inceptionv3	32	1,296.16	1,296.14	0.00%	✅
torchvision-inceptionv3_fp16	32	2,539.40	2,534.23	0.20%	✅
cadene-inceptionv4	16	620.04	620.23	-0.03%	✅
cadene-resnext64x4	16	588.33	588.45	-0.02%	✅
slim-mobilenet	64	7,211.84	7,215.95	-0.06%	✅
slim-nasnetalarge	64	236.16	236.51	-0.15%	✅
slim-resnet50v2	64	2,553.53	2,555.59	-0.08%	✅
bert-mrpc-onnx	8	824.43	825.02	-0.07%	✅
bert-mrpc-tf	1	389.33	389.38	-0.01%	✅
pytorch-examples-wlang-gru	1	297.55	294.47	1.04%	✅
pytorch-examples-wlang-lstm	1	320.15	307.96	3.96%	🔆
torchvision-resnet50_1	1	550.81	544.78	1.11%	✅
torchvision-inceptionv3_1	1	304.72	305.19	-0.16%	✅
cadene-dpn92_1	1	353.21	352.21	0.28%	✅
cadene-resnext101_1	1	219.81	220.12	-0.14%	✅
slim-vgg16_1	1	224.17	224.29	-0.06%	✅
slim-mobilenet_1	1	1,499.85	1,497.40	0.16%	✅
slim-inceptionv4_1	1	216.44	214.96	0.69%	✅
onnx-taau-downsample	1	305.80	306.79	-0.32%	✅
dlrm-criteoterabyte	1	21.66	21.67	-0.05%	✅
dlrm-criteoterabyte_fp16	1	40.71	40.71	-0.01%	✅
agentmodel	1	5,836.49	5,776.01	1.05%	✅
unet_fp16	2	55.12	55.16	-0.07%	✅
resnet50v1_fp16	1	755.40	766.51	-1.45%	✅
bert_base_cased_fp16	64	970.85	971.03	-0.02%	✅
bert_large_uncased_fp16	32	304.94	305.05	-0.04%	✅
bert_large_fp16	1	167.12	166.83	0.17%	✅
distilgpt2_fp16	16	1,352.61	1,350.80	0.13%	✅

Check results before merge 🔆

migraphx-bot · 2023-09-06T14:19:21Z

:white_check_mark:bert-mrpc-onnx: PASSED: MIGraphX meets tolerance

:white_check_mark:bert-mrpc-tf: PASSED: MIGraphX meets tolerance

:white_check_mark:pytorch-examples-wlang-gru: PASSED: MIGraphX meets tolerance

:white_check_mark:pytorch-examples-wlang-lstm: PASSED: MIGraphX meets tolerance

:white_check_mark:torchvision-resnet50_1: PASSED: MIGraphX meets tolerance

:white_check_mark:torchvision-inceptionv3_1: PASSED: MIGraphX meets tolerance

:white_check_mark:cadene-dpn92_1: PASSED: MIGraphX meets tolerance

:white_check_mark:cadene-resnext101_1: PASSED: MIGraphX meets tolerance

:white_check_mark:slim-vgg16_1: PASSED: MIGraphX meets tolerance

:white_check_mark:slim-mobilenet_1: PASSED: MIGraphX meets tolerance

:white_check_mark:slim-inceptionv4_1: PASSED: MIGraphX meets tolerance

:white_check_mark:dlrm-criteoterabyte: PASSED: MIGraphX meets tolerance

:white_check_mark:agentmodel: PASSED: MIGraphX meets tolerance

:white_check_mark:unet: PASSED: MIGraphX meets tolerance

:white_check_mark:resnet50v1: PASSED: MIGraphX meets tolerance

🔴bert_base_cased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output

🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output

:white_check_mark:bert_large: PASSED: MIGraphX meets tolerance

🔴distilgpt2_fp16: FAILED: MIGraphX is not within tolerance - check verbose output

codecov · 2023-09-06T15:06:19Z

Codecov Report

Merging #2155 (6b7e804) into develop (9316c00) will not change coverage.
The diff coverage is n/a.

❗ Current head 6b7e804 differs from pull request most recent head 8d1157c. Consider uploading reports for the commit 8d1157c to get more accurate results

@@           Coverage Diff            @@
##           develop    #2155   +/-   ##
========================================
  Coverage    91.50%   91.50%           
========================================
  Files          431      431           
  Lines        16140    16140           
========================================
  Hits         14769    14769           
  Misses        1371     1371

see 2 files with indirect coverage changes

pfultz2 · 2023-09-06T15:06:29Z

test/msgpack.cpp

@@ -76,7 +76,7 @@ TEST_CASE(test_msgpack_float)
    migraphx::value v = 3.0;
    auto buffer       = migraphx::to_msgpack(v);
    EXPECT(buffer == msgpack_buffer(3.0));
-    EXPECT(migraphx::from_msgpack(buffer) == v);
+    EXPECT(migraphx::from_msgpack(buffer).to<float>() == v.to<float>());


Why does this need to be converted to float first?

Function migraphx::from_msgpack returns value which needs to be converted to other types (except for string and bool case) when we are comparing output values. The tests above convert value to appropriate type (here is the example for int).

pfultz2 · 2023-09-06T15:08:30Z

test/msgpack.cpp

-    EXPECT(migraphx::from_msgpack(buffer) == v);
+
+    // converted to vector in the following line because in value.cpp value constructor with
+    // unordered map is creating vector<value> with map items as vector elements


Where are we using an unordered map?

Created buffer is a map. After its creation there is check if buffer is equal with buffer initialized with map (here).
In value definition inside value.hpp I haven't seen constructor for map, I found just constructor for unordered map, so because of that I used conversion to vector.

In value definition inside value.hpp I haven't seen constructor for map, I found just constructor for unordered map

Its not using the std::unordered_map constructor, its using the std::initializer_list constructor which will preserve the order of the list.

I saw value constructor for std::initializer_list in value.cpp. In this case (where size is not 2 and elements have key) it is calling set_vector function, so I think to_vector() function can be used for comparing if result values are equal.

to_vector will only compare the values and not the keys. The == operator for value will compare the keys as well: migraphx::from_msgpack(buffer) == v.

If migraphx::from_msgpack(buffer) function is fixed, I won't have need to cast output to float in the following way which i committed in this PR too (EXPECT(migraphx::from_msgpack(buffer).to<float>() == v.to<float>());).

but the problem here is that migraphx::from_msgpack(buffer) converts any numeric value to uint64_t

Where do you see this happening? from_msgpack converts the float32 or float64 to double here: https://github.com/ROCmSoftwarePlatform/AMDMIGraphX/blob/develop/src/msgpack.cpp#L85.

I see it in the debugger during comparing EXPECT(migraphx::from_msgpack(buffer) == v);. You can see debugger output in the following screenshot when debugging test_msgpack_float, here x is migraphx::from_msgpack(buffer) output and y is migraphx::value v which is double. If we cast v to be uint64_t, the test passes.
Also, if there is type conversion why do we need to cast function output to int here (which already exists in develop branch)?

My suggestion since we do want to keep comparing the keys and the types are different for the values that we should either change operator== to have a specialization that can properly compare types, call the compare function directly if that will allow us to compare types or create a compare function that can compare the keys and compare with an explicit type conversion (passed as a template parameter).

So there is something wrong with either msgpack serialization or the value construction as it should be float_type as well.

test/msgpack.cpp

tvukovic-amd · 2023-10-05T09:44:16Z

I changed code without cast msgpack output to expected type according to following findings on Windows (tests pass on Linux): the thing here is that migraphx::to_msgpack(v) and msgpack_buffer(3.0) from msgpack.cpp file which is used for this test (3.0 is some double value) both call msgpack::pack(vs, v); function inside AMDMIGraphX\build\_deps\msgpack-src\include\msgpack\v1\pack.hpp on Windows.

That function is calling msgpack::operator<<(*this, v); which after couple of steps calls pack_double function inside the same pack.hpp file with following conditions:

template <typename Stream>

inline packer<Stream>& packer<Stream>::pack_double(double d)

{

    if(d == d) { // check for nan 

        // compare d to limits to avoid undefined behaviour

        if(d >= 0 && d <= double(std::numeric_limits<uint64_t>::max()) && d == double(uint64_t(d))) {

            pack_imp_uint64(uint64_t(d));

            return *this;        

        } else if(d < 0 && d >= double(std::numeric_limits<int64_t>::min()) && d == double(int64_t(d))) {

            pack_imp_int64(int64_t(d));

            return *this;

        }

    }



    union { double f; uint64_t i; } mem;

    mem.f = d;

    char buf[9];

    buf[0] = static_cast<char>(0xcbu);



#if defined(TARGET_OS_IPHONE)

    // ok

#elif defined(__arm__) && !(__ARM_EABI__) // arm-oabi

    // https://github.com/msgpack/msgpack-perl/pull/1

    mem.i = (mem.i & 0xFFFFFFFFUL) << 32UL | (mem.i >> 32UL);

#endif

    _msgpack_store64(&buf[1], mem.i);

    append_buffer(buf, 9);

    return *this;

}

The thing here is that when we have double input which is same as d == double(int64_t(d)) and is in appropriate range, for example (1.0, 3.0 etc.), first or second conditions pass (inside d==d condition) from code above and we get result from pack function which is uint64_t or int64_t type because here it is converted to it and it is not double type which is expected to be.

If I change inputs inside test code for example from 3.0 to 3.01 or whatever value which is not same as value after conversion to int64_t or uint64_t and then to double, all tests pass because conditions from code above are not met and we have returned double value which is expected. If we leave 3.0 it will be uint64 or int64 and because of that won't pass the test since it doesn't pass type check since their types are different.

Added fix for test_msgpack_float and test_msgpack_object

f557e67

causten requested a review from umangyadav September 6, 2023 14:06

pfultz2 reviewed Sep 6, 2023

View reviewed changes

apwojcik added the Windows Related changes for Windows Environments label Sep 6, 2023

tvukovic-amd and others added 2 commits September 27, 2023 19:26

Added warning fix for double equal

3c400ba

Merge branch 'develop' into fix/msgpack_test

ec541ae

apwojcik requested a review from pfultz2 September 28, 2023 14:30

pfultz2 reviewed Sep 28, 2023

View reviewed changes

test/msgpack.cpp Outdated Show resolved Hide resolved

tvukovic-amd and others added 5 commits September 29, 2023 09:30

Added test::within_abs in comparing float values

bf42d61

Added test::within_abs in comparing float values

959a01c

Merge branch 'develop' into fix/msgpack_test

22a9ea0

Merge branch 'develop' into fix/msgpack_test

f391728

Added Windows fix for double inputs in test

9b5a580

tvukovic-amd added 2 commits October 5, 2023 12:03

Added Windows fix for double inputs in test

cea31cc

Added comments why double inputs are changed

d2fd5ac

umangyadav approved these changes Oct 5, 2023

View reviewed changes

pfultz2 approved these changes Oct 6, 2023

View reviewed changes

Merge branch 'develop' into fix/msgpack_test

8d1157c

causten merged commit f3f6598 into develop Oct 6, 2023
8 of 9 checks passed

causten deleted the fix/msgpack_test branch October 6, 2023 22:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added fix for test_msgpack_float and test_msgpack_object #2155

Added fix for test_msgpack_float and test_msgpack_object #2155

tvukovic-amd commented Sep 6, 2023

migraphx-bot commented Sep 6, 2023 •

edited

Loading

migraphx-bot commented Sep 6, 2023 •

edited

Loading

codecov bot commented Sep 6, 2023 •

edited

Loading

pfultz2 Sep 6, 2023

tvukovic-amd Sep 8, 2023

pfultz2 Sep 6, 2023

tvukovic-amd Sep 7, 2023

pfultz2 Sep 8, 2023

tvukovic-amd Sep 21, 2023

pfultz2 Sep 28, 2023

tvukovic-amd Sep 29, 2023 •

edited

Loading

pfultz2 Oct 2, 2023

tvukovic-amd Oct 3, 2023 •

edited

Loading

tperry-amd Oct 3, 2023

pfultz2 Oct 3, 2023

tvukovic-amd commented Oct 5, 2023

Added fix for test_msgpack_float and test_msgpack_object #2155

Added fix for test_msgpack_float and test_msgpack_object #2155

Conversation

tvukovic-amd commented Sep 6, 2023

migraphx-bot commented Sep 6, 2023 • edited Loading

migraphx-bot commented Sep 6, 2023 • edited Loading

codecov bot commented Sep 6, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvukovic-amd Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvukovic-amd Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvukovic-amd commented Oct 5, 2023

migraphx-bot commented Sep 6, 2023 •

edited

Loading

migraphx-bot commented Sep 6, 2023 •

edited

Loading

codecov bot commented Sep 6, 2023 •

edited

Loading

tvukovic-amd Sep 29, 2023 •

edited

Loading

tvukovic-amd Oct 3, 2023 •

edited

Loading