Submission checker modularization

[pgmpablo157321]

#1670
Testing command (outside the inference repo):

python -m inference.tools.submission.submission_checker.main --input inference_results_v5.1

implemented checks

performance:

• Check performance detailed log exists
• Check for loadgen errors
• Check for equal issue mode when it is required
• Check the performance sample count used for running the benchmark
• Check loadgen seeds are correct
• Check latency constrain is met
• Check minimun query count is met
• Check minimun duration is met
• Check network requirements
• Check LLM latencies are met (if applies)
• Check loadgen scenario matches with submission scenario or that result can be inferred

accuracy

• Check the accuracy metric is correct and over the expected threshold (or within a range if applies)
• Check accuracy json exists and is truncated
• Check for loadgen error
• Check full dataset is used for the accuracy run

compliance

• Check compliance directory exists
• Run performance checks for compliance run
• Check accuracy test passes
• Check performance test passes

measurements

• Check measurements files exist
• Check the required files are there
• Check the required fields are there

power

• Check the required power files are there (if the submission has power)
• Run the external power checks
• Check power metric can be calculated

system

• Check system json exists
• Check availability is valid
• Check system type is valid
• Check network fields
• Check required fields are include in system json file
• Check submitter is correct
• Check division is correct

[github-actions]

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

[nv-alicheng]

We can utilize the __init_subclass__ feature of Python to handle this registry-like functionality.

https://peps.python.org/pep-0487/#subclass-registration

I believe a better paradigm would be something like:

1. Implement a Checker subclass that inherits BaseChecker
2. Each Checker class will implement some number of methods that are prefixed with check_
3. BaseChecker's __call__ and execute() methods will check all attributes on the class and run (in sequence) all attributes that are callable and start with the string check_
4. If there is a dependency, we can implement an @BaseCheck.mark_dependency(<str>, ...) decorator where you can pass in a list of strings that are the function names, which need to be executed before the current check.

From what I could tell, all the implemented Check classes have an init method with the arguments log, path, config, submission_logs - The BaseCheck class should probably do the same and just store the values in self to be used by the subclasses later.

[nv-alicheng]

I'm wondering if it makes more sense to run every check here and return the success value of each, keyed by the checker's class? Something like:

{
    AccuracyCheck: True,
    ComplianceCheck: False,
    PerformanceCheck: True,
    ...
}

I can see this being clunky if many tests depend on each other, which means that check failures will cascade. In which case my question is should there be a system to determine the dependencies of each test? Something like MeasurementsCheck depends on DirectoryStructureCheck, etc.

[nv-alicheng]

Does it make more sense to have a ComplianceCheck class (which inherits BaseCheck), then have individual TEST0XCheck subclasses?

[nv-alicheng]

Can you add a formatter to the project like autopep8 or black?

[nv-alicheng]

The overloading of the term log here feels clunky and confusing. A few comments here:

1. It seems like bad practices to create a logger named 'main' and pass it around to each Checker. It would be better that each file has it's own logger (logging.getLogger(__file__)) so that if (for instance) ExampleCheck did log.info("Missing file ___"), the message in console would show that it originated in ExampleCheck rather than main.
2. If each file has its own logger, you no longer need to pass around log everywhere (makes it more concise)
3. If we are passing in logs, is there a point to also pass in logs.loader_data[key]? Can't that just be extracted by the Check's init method?
4. If this is simplified down to just xxxx_check = XXXXCheck(config, logs), then it can further be simplified down to

for logs in loader.load():
    for check_cls in [PerformanceCheck, ...]:
        check_cls(config, logs)()

[nv-alicheng]

Is this a TODO?

[nv-alicheng]

Why are these empty files here?

[nv-alicheng]

I mentioned this in the GH Issue, but if the giant model dict is already being stored in a Python file, it should probably be refactored into some hierarchy of dataclasses. Having a class based representation would also make doing the key -> property remapping you're doing here either easier or unnecessary.

Having it as dataclasses rather than a dict also makes the schema of the config more defined and easier to navigate.

[anandhu-eng]

Hi @pgmpablo157321 , could you please update the Summary section of the README to reflect what the updated command looks like?

[mkankana]

initial test with 5.1 submission seem to be passing.

[mrmhodak]

@nv-alicheng @nvzhihanj to take a look and provide guidance whether to merge for 6.0

[hanyunfan]

@arjunsuresh Hi Arjun, I’m not sure if you’re the right person for this. Could you, or someone else from the automation group, help address the ResNet50 and RetinaNet errors in some of the checks? It looks like this is affecting multiple PRs, which aren't related with ResNet and RetinaNet at all.

[nvzhihanj]

@pgmpablo157321 Thanks for the PR! it seems like some of Alice's comments are not addressed yet. I would suggest that we either address them in this PR, or document the TODOs (in the issue) and address in follow-up PRs.
Some of the refactors should be quite easy with Cursor I believe

[hanyunfan]

Pablo confirm this is ready for merging.

[hanyunfan]

WG: It is ok to merge it.

[hanyunfan]

LGTM