Add CLI option to directly pass reference files when running tests #28446
Conversation
Maxim-Doronin
added
good first issue
| @@ -127,6 +127,7 @@ DEFINE_string( | |||
| ref_dir, | |||
| "", | |||
| "A directory with reference blobs to compare with in run_test mode. Leave it empty to use the current folder."); | |||
| DEFINE_string(ref_results, "", "Reference files used during 'run_test'"); | |||
There was a problem hiding this comment.
Please, specify input format in case of several inputs.
Is it --ref_results "file1;file2"?
Are relative paths supported? e.g. --ref_dir /part/of/path/ --ref_results rest/of/path/file.blob?
Are absolute paths supported? e.g. --ref_results /abs/path/file.blob
| // Make sure that the number of reference files is the same as the number of input files | ||
| if (refFilesPerCase.size() != inputFilesPerCase.size()) { | ||
| std::cout << "The number of reference files is not equal to the number of input files." | ||
| << " # Reference Files: " << refFilesPerCase.size() |
There was a problem hiding this comment.
Please replace # with number of or # of
| } | ||
|
|
||
| for (const auto& refResult : refFilesPerCase) { | ||
| std::vector<std::string> refFilesPerModel = splitStringList(refResult, ','); |
There was a problem hiding this comment.
AFAIR, --network supports only a single model to be loaded. Please explain when we expect several models in a single SIT run.
There was a problem hiding this comment.
I guess that it is for batched input scenario when a whole row looks like CASE_0;CASE_1;etc
where
CASE_X is a run of SIT and which contains INPUT_0,INPUT_1,INPUT2,etc,
where
INPUT_0 encompasses data for single input in batch BATCH_0|BATCH_1|etc
which makes sense for separation input data per model
If my understanding is correct that here we need to produce tensors which are belong to output data types. In case of batch, though inputs pictures for every batch line are separated, outputs are printed into unified single file without separation of its lines. So probably we should not use this level with '|' as perFiles separator
| } | ||
|
|
||
| for (const auto& refResult : refFilesPerCase) { | ||
| std::vector<std::string> refFilesPerModel = splitStringList(refResult, ','); |
There was a problem hiding this comment.
I guess that it is for batched input scenario when a whole row looks like CASE_0;CASE_1;etc
where
CASE_X is a run of SIT and which contains INPUT_0,INPUT_1,INPUT2,etc,
where
INPUT_0 encompasses data for single input in batch BATCH_0|BATCH_1|etc
which makes sense for separation input data per model
If my understanding is correct that here we need to produce tensors which are belong to output data types. In case of batch, though inputs pictures for every batch line are separated, outputs are printed into unified single file without separation of its lines. So probably we should not use this level with '|' as perFiles separator
| if (!refFiles.empty()) { | ||
| blobFileFullPath = refFiles[numberOfTestCase][outputInd]; | ||
| } else { | ||
| std::ostringstream ostr; |
There was a problem hiding this comment.
nitpick:
I'd rather move this into a separate function like 'generate_reference_file_name(...)', which will makes this huge code lesser overwhelming
There was a problem hiding this comment.
nitpick 2:
I wondered whether forming the array refFiles initially regardless either ref_dir or res_output set and stick to it will without this condition checking refFiles.empty() would be more nice or not.
So that we could fill refFiles by using generate_reference_file_name() (because all data to generate is known in the beginning) when ref_dir exist or just parse command line of res_output
Please feel free to ignore this comment as it's arguable
| const auto blobFileFullName = fullPath.string(); | ||
| // If reference files are provided, use them | ||
| if (!refFiles.empty()) { | ||
| blobFileFullPath = refFiles[numberOfTestCase][outputInd]; |
There was a problem hiding this comment.
I found that we have already taken numberOfTestCase here https://github.com/openvinotoolkit/openvino/pull/28446/files#diff-32f65067f63ae981b516b3780122e243b0ed1684efd43992f00063f0d458716eR2118
when we had defined refFiles by itself. Probably when use remove '|' parsing level this second numberOfTestCase would not required anymore
| : refFilesForOneInfer[numberOfTestCase]; | ||
| if (!FLAGS_ref_results.empty()) { | ||
| OPENVINO_ASSERT(refFiles.size() == outputsInfo.size(), "Number of reference files ", refFiles.size(), | ||
| " doesn't match network configuration ", outputsInfo.size()); |
There was a problem hiding this comment.
+100500 for printing troubleshooting info's!
I'd only suggest emphasize that it must match output count by amending this doesn't match network output count: ', outputsInfo.size()
| std::cout << " Reference files direcotry: " | ||
| << (FLAGS_ref_dir.empty() ? "Current directory" : FLAGS_ref_dir) << std::endl; | ||
| std::cout << " Reference files directory: " | ||
| << (FLAGS_ref_dir.empty() && FLAGS_ref_results.empty() ? "Current directory" : FLAGS_ref_dir) |
There was a problem hiding this comment.
from a context it sounds like these parameters are mutually exclusive so that I presume that printing a message and exiting with an error is more intentional
Details:
<netname>_ref_out....blob) is used.Tickets: