Refactor state.dart (part 1)
#591
Conversation
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
|
@anhappdev I think it would be better if you leave your comments, but approve this PR, and then I'll fix it in the next PR for #556. |
| @@ -1,6 +1,6 @@ | |||
| import 'package:mlperfbench_common/data/results/benchmark_result.dart'; | |||
|
|
|||
| class RunResult { | |||
| class NativeRunResult { | |||
There was a problem hiding this comment.
I think adding a brief description for each of the class as a comment will be helpful since the class name some time is not descriptive enough.
| final String mode; | ||
| final String logSuffix; | ||
| final String loadgenMode; | ||
| final String readable; |
There was a problem hiding this comment.
What does readable mean here? Do you mean description?
There was a problem hiding this comment.
Value of this variable should be human-readable.
Previously it was used only in log dir name (which is intended to be handled by vendor developers), now I also use as a log prefix.
Value should be short, to consume less screen space, and it should look nice.
I don't think description would fit here, because I would expect description to be more than just one word.
There was a problem hiding this comment.
Can you add a comment and example value to explain it? I still not understand what is it and for what it's used?
There was a problem hiding this comment.
All possible values are already in this file: performance and accuracy, just few lines above.
I think they are pretty self-explanatory.
Example usage:
log dir name: image_classification_offline-performance
live logs: image_classification: performance mode: starting
There was a problem hiding this comment.
I think it would be better to change the name than adding comments with example usage.
But I can't come up with a good name.
If you also don't have any fitting suggestions, I'll add a comment in next PR.
There was a problem hiding this comment.
So, just like it was before this PR :-)
I renamed the field because I thought logSuffix kinda doesn't fit the fact that it is no longer a true suffix, and the content of the variable will look fitting in any text.
OK, I'll revert it.
There was a problem hiding this comment.
I only see it used as a log dir suffix in the code, so that name would be good for now.
There was a problem hiding this comment.
live logs:
image_classification: performance mode: starting
Not a log dir suffix
There was a problem hiding this comment.
The only place I see it used is logDir = '$logParentDir/${benchmark.id}-${runMode.readable}'
There was a problem hiding this comment.
mobile_app_open/flutter/lib/benchmark/run_mode.dart
Lines 43 to 45 in 5312c76
| } | ||
|
|
||
| Future<LoadgenInfo?> extractLoadgenInfo() async { | ||
| const logFileName = 'mlperf_log_detail.txt'; |
There was a problem hiding this comment.
I think we can make all string const like this static const.
There was a problem hiding this comment.
const keyword in Dart is not like const in C++. It's already static const, because in Dart all const values use static initialization, and can only have one instance.
static const varName in code will result in compilation error, because it's not a valid syntax in Dart for local fields (for class fields it's the other way around).
There was a problem hiding this comment.
Yeah, static const mean the same as class const in some other languages, so it needs to be defined as a class field. Normally, I would either define this kind of string as a global constant, or a class constant, since it is kind of setting.
There was a problem hiding this comment.
OK, I'll move it to class level
d-uzlov
force-pushed
the
556/refactor-state-1
branch
from
7e28ef9 to
71b8438
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
@anhappdev could you approve this, please? |
Part of #556
In this PR I extract several helper classes from
BenchmarkState:TaskRunner,ResultHelper,ValidationHelper.Overall the layout of classes and methods is still suboptimal, but I want to merge this now to reduce possible merge issues.
I have already done manual merge/rebase
3 times4 times because literally any changes tostate.dartin other PRs require manual merge with this one. And almost any changes in the app require a change in thestate.dart.