KhronosGroup / NNEF-Tools

The NNEF Tools repository contains tools to generate and consume NNEF documents
https://www.khronos.org/nnef
222 stars 57 forks source link

quant file processing #75

Closed jnorwood closed 5 years ago

jnorwood commented 5 years ago

I noticed that the current parser no longer parses the quant file. That's a pretty big change without a notice. It was convenient for code generation to have all the quantization info coming over in one file. Now the code generation program will have to read many binary files to get compile-time constants for scaling into the sources. Was there some discussion of this?

gyenesvi commented 5 years ago

This is not an intentional change, it should be working with the same logic as previously. Which interface are you using, C++ or Python?

jnorwood commented 5 years ago

I checked out the parser code, built it, tried to parse some previous examples that had quant file, but there is no longer a --quant option in main. Is it no longer an option?

gyenesvi commented 5 years ago

Oh, okay, so the main has changed, the way it works is that if you give it an NNEF folder, it parses everything, including weights and quantization file, but if you give it a single graph file it only parses the graph.

The interface of the parser changed quite a bit in the past updates (as you may noticed it returns the result in a clean structure). If you are building on this code and need to adjust some code you may want to wait a bit, another update is coming early next week that cleans up things a bit more.

jnorwood commented 5 years ago

I see a simplified quant file listed in the spec, with a min, max, and bits parameter.
I have been specifying a zero offset value. How should symmetrical vs asymmetrical quantization be distinguished?

jnorwood commented 5 years ago

Ok, I'll wait a few days. I saw that all the callbacks had been removed. That's probably easier now.

gyenesvi commented 5 years ago

In the graph file, you can define your own quantization operation that has a zero point and a scale parameter instead of the min and max, if you want. And then you can use that in the quant file. Does that solve your problem?

jnorwood commented 5 years ago

Yes, I have been creating my own quantization operation, but 5.3 in the spec doesn't make clear that you can still do that.

gyenesvi commented 5 years ago

Yes, you can still do that, there was no change in that regard.

gyenesvi commented 5 years ago

The parser code has been updated, along with the readme that describes the command line arguments. If you only use the parser for validation of the NNEF files, I'd suggest using the Python script (validate.py) in the python folder.

jnorwood commented 5 years ago

The CMakeLists.txt add_executable line is broken. It needs the nnef.cpp source, as previously done.

add_executable(${target} ${LIB_HEADERS} main.cpp nnef.cpp)

jnorwood commented 5 years ago

There is another bug:

1>c:\nnef6\nnef-tools\parser\common/shapes.h(893): error C4576: a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax

fixed by:

Shape C(input[0], filter[0]); return C; //return (Shape){ input[0], filter[0] };

After that, the cpp parser builds and passes a test with the vgg.txt from parser/examples. I'm building this with MSVC ... c:\nnef6\build>cmake -A x64 ..\NNEF-Tools\parser

jnorwood commented 5 years ago

The "--shapes" option is also broken, causing a crash in infer_shapes in nnef.cpp if custom_shapes is empty. Fixed by checking for custom_shapes.empty() before use:

` auto it = StandardShapeFuncs.find(op.name);

        if ( it == StandardShapeFuncs.end() )
        {

            if (!custom_shapes.empty())
            {

                it = custom_shapes.find(op.name);

            }

        }
        if ( it == StandardShapeFuncs.end() || (!custom_shapes.empty() && it == custom_shapes.end() ))
        {

            error = "Shape function for operation '" + op.name + "' is not provided";
            return false;

        }

`

jnorwood commented 5 years ago

I also want to ask if the output from validate should also be a valid graph. It currently clips off the version statement and the expansion of the tensor shapes with the --shapes option is also rejected by the parser. Also, it would help if the pass/fail status would go to stderr instead of stdout , since it currently gets added to the end of the graph.

gyenesvi commented 5 years ago

I have fixed the CMakeList.txt, and the bug about the return statement. By the way, I believe your fix was wrong, as it calls the wrong constructor of std::vector, this should work:

return Shape({ input[0], filter[0] });
gyenesvi commented 5 years ago

I have also submitted a fix to the --shapes problem, I think the problem was not that the custom_shapes was empty (it is okay to search an empty map), but that I was potentially comparing iterators of two different maps. Please check if it runs well now!

gyenesvi commented 5 years ago

About the output of the main, it is not supposed to be a valid NNEF stream, it is not so simple to make it always valid, since if there are custom operation definitions, those would have to be printed back as well. This output is more for debugging purposes, that is why it inserts the shape info as well.

What I did is that I redirected all errors and accept prints to std::cerr, furthermore put the shape information to the end of the line as a comment instead. This way, the output that is printed to std::cout should be legal NNEF, albeit incomplete, it only contains the graph definition.

jnorwood commented 5 years ago

ok, thanks. I'll check these out.

jnorwood commented 5 years ago

Ok, the changes built and evaluated the examples/vgg.txt without error.
I have a question about the inconsistencies in the padding expansion, using examples/vgg.txt as the test case. for conv17, the padding values are all 0, and are expanded. for conv18, the padding values are shown as empty.

Is there difference of these two convolutions that is implied by the difference in the padding expansion?

gyenesvi commented 5 years ago

Padding with zeros means 'valid' padding basically. The empty array means 'same' padding, i.e. the padding values are automatically calculated so that the output has the same (or downscaled in case of striding) size as the input.

jnorwood commented 5 years ago

From the spec, Padding is automatically calculated from the other parameters in all dimensions where it is not specified. So, if the parser is calculating and filling in padding in the internal data structure, should we have a way to verify what was calculated, as you provided with the --shapes option?

gyenesvi commented 5 years ago

Well, actually it is not needed to be calculated. It is 'as if' it was calculated and the substituted into the general formula for output shape. In general, providing the padding is required to specify the exact output shape. But when you don't provide padding (it means auto-padding is used), the output shape is calculated in a simpler way, such that it is the same as the input shape (or simply down sampled version of it). So if you have the padding you can calculate the output shape, but if you have the output shape (= input shape / stride) then you could calculate the corresponding padding, but you don't need it, you only need the output shape. So you can verify that by looking at the output shape itself.

jnorwood commented 5 years ago

Ok, but according to the spec in the section "Automatic Padding Calculation", if the padding array is empty, it will be automatically calculated. So, in vgg17.txt example, I changed conv17 padding to empty, yet there was no apparent padding calculation resulting in the parameters or in the display of the padding.

I changed it to: conv17 = conv(pool5, kernel17, bias17, padding = [], border = 'constant', stride = [1, 1], dilation = [1, 1]); from conv17 = conv(pool5, kernel17, bias17, padding = [(0, 0), (0, 0)], border = 'constant', stride = [1, 1], dilation = [1, 1]); So, I don't see any evidence that there was an Automatic Padding Calculation performed, as stated in the description.

I thought, maybe I would see some evidence of an auto calculation if non-zero values were required, so I ran it again with the padding of conv10 changed from .padding = [(1, 1), (1, 1)] to padding = [], but I don't see any evidence of an auto calculation by the parser.

So, does the empty array mean the padding calculation is expected to be done by the user's code?

gyenesvi commented 5 years ago

In the VGG example, ac conv17, the input size should be 7x7, and the filter size of conv17 is also 7x7, so with padding with zeros, you should get an output size of 1x1. However, if you change it to auto-padding, because stride is 1x1, you should get an output size of 7x7 (like for 'same' padding in TF); that's the evidence. In this case, it is as if the padding was actually 3x3. But to calculate the output size, you don't need to calculate the actual padding. If the consumer code needs that actual value of padding for the implementation of the conv, it needs to calculate the padding according to the formula in the Automatic Padding section. So the text "it will be automatically calculated.." actually means "it shall be calculated by the consumer if required..".

The point of auto-padding is that it makes the description input-shape independent in the sense that the same description works for all input shapes. The padding is automatically adjusted to accommodate the 'output-shape = input-shape / stride' constraint. In strided cases this may require asymmetric paddings which may have different values for different input sizes, so a fixed padding value would not work.

jnorwood commented 5 years ago

attached is a zip that holds the CMakeLists.txt file that downloads and converts 38 tflite and onnx models (converts to nnef). It also hsa a results.txt file that shows the results from running ctest. This currently runs in NNEF-Tools. It puts results in models_tflite and models_onnx. It is using cmake 3.14.2, so you need to build cmake from sources and install that. Make sure you have libssl-dev installed before building cmake, since it needs that to download files from https URLs. ` Start 76: validate_vgg19-bn 76/76 Test #76: validate_vgg19-bn ....................... Passed 0.01 sec

100% tests passed, 0 tests failed out of 76

Total Test time (real) = 309.92 sec `

convert_cmake.zip

jnorwood commented 5 years ago

It would be better to be able to run that from an external directory, but that would require installing some local modules that are used by the NNEF python conversion scripts.

gyenesvi commented 5 years ago

Thanks for contributing this, however, it is not clear to me what is the purpose of this CMakeLists.txt. Does this run every time you run cmake? The validation tests would be useful, but we definitely don't want to introduce the above dependencies and slow down the build process with such tests. Or can this be run independently from the main build?

jnorwood commented 5 years ago

It runs if you execute cmake with that directory as the source directory.

cd NNEF-Tools
cmake .
ctest

Currently it is also adding the parser's build as a sub-directory, since it uses the parser in the tests.

Normally this would go in a test directory, and you should be able to run it from outside the repository tree. However, I think there needs to be an install script for all the python conversion modules before that can be done. I didn't find one, but maybe I missed it.

So, for now, if someone wants to do all those conversions, they just need to add that CMakeLists.txt to the NNEF-Tools folder, and execute cmake in that same folder, with the commands above. The results will go in models_tflite and models_onnx, under NNEF-Tools.

jnorwood commented 5 years ago

The downloads occur the first time you run cmake, and are kept in a _deps subdirectory. The conversions are executed by either ctest or by make test. I only tested ctest.