Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Good First Issue][NNCF]: fix invalid error reporting in JSON schema #2570

Open
ljaljushkin opened this issue Mar 11, 2024 · 19 comments
Open
Labels
good first issue Good for newcomers

Comments

@ljaljushkin
Copy link
Contributor

Context

Configuration for NNCF algorithms can be defined in JSON format. For instance, [config.json].(https://github.com/openvinotoolkit/nncf/blob/develop/examples/torch/classification/configs/sparsity_quantization/inception_v3_imagenet_rb_sparsity_int8.json#L17-L38)

Compression section in these configuration files are validated using jsonschema.validate

But there's some bug with reporting error in JSON schema when a field is not defined in schema.

Steps to reproduce:

  1. checkout https://github.com/ljaljushkin/nncf_pytorch/tree/nl/bug_in_schema_report

  2. There's unit test checking 2 config files:

To run tests:

test $pytest tests/torch/test_config_schema.py -sv -k 'bug_in_error_report'

The expected behavior, that the first test should pass, the second — fail.
But the reported error in the second test should be the following:

error = <ValidationError: "Additional properties are not allowed ('unknown_field' was unexpected)">

However, the second test reports this, which is not correct:

jsonschema.exceptions.ValidationError: 'polynomial' is not one of ['exponential', 'exponential_with_bias', 'baseline'] 

The message is misleading, because it's saying that the supported schedule type polynomial for rb_sparsity doesn't correspond to the list of supported schedule types for the filter pruning algorithm.

https://openvinotoolkit.github.io/nncf/schema/index.html#compression_oneOf_i0_oneOf_i1_params_schedule
https://openvinotoolkit.github.io/nncf/schema/index.html#tab-pane_compression_oneOf_i0_oneOf_i9

filter pruning's definition: https://github.com/openvinotoolkit/nncf/blob/develop/nncf/config/schemata/algo/filter_pruning.py#L88
rb sparsity's definition:
https://github.com/openvinotoolkit/nncf/blob/develop/nncf/config/schemata/common/sparsity.py#L52C14-L52C30

What needs to be done?

Fix the error reporting for the given test case

Example Pull Requests

The issue is not reproduced before these changes:
Refactor schemata and prepare for rendering. (#1268)

Resources

Here's the automatically generated page with parameters from json schema.
https://openvinotoolkit.github.io/nncf/schema/index.html#compression

Contact points

@ljaljushkin @vshampor

Ticket

94933

@ljaljushkin ljaljushkin added the good first issue Good for newcomers label Mar 11, 2024
@LalitSP
Copy link

LalitSP commented Mar 11, 2024

.take

Copy link

Thank you for looking into this issue! Please let us know if you have any questions or require any help.

@LalitSP
Copy link

LalitSP commented Mar 18, 2024

Hi @ljaljushkin @vshampor, I've been working on this issue for the last few days and I would appreciate your assistance. Could you please help me identify the source of the error? I need to know if it's originating from jsocschema.validate or from sparsity.py.

@ljaljushkin
Copy link
Contributor Author

Hi @LalitSP. I suppose the error is not in the jsonschema package, but rather from the way we define schema for sparsity, because this error wasn't reproduced before: Refactor schemata and prepare for rendering. (#1268)

@LalitSP

This comment was marked as outdated.

@ljaljushkin
Copy link
Contributor Author

How if we modify validate_single_compression_algo_schema function?

def validate_single_compression_algo_schema(single_compression_algo_dict: Dict, ref_vs_algo_schema: Dict): """single_compression_algo_dict must conform to BASIC_COMPRESSION_ALGO_SCHEMA (and possibly has other algo-specific properties""" algo_name = single_compression_algo_dict["algorithm"] if algo_name not in ref_vs_algo_schema: raise jsonschema.ValidationError( f"Incorrect algorithm name - must be one of {str(list(ref_vs_algo_schema.keys()))}" ) try: jsonschema.validate(single_compression_algo_dict, schema=ref_vs_algo_schema[algo_name]) except jsonschema.ValidationError as e: if e.validator == 'additional_properties': e.message = f"Unknown field found in '{algo_name}' configuration: {e.instance}" else: # Handle other validation errors e.message = ( f"While validating the config for algorithm '{algo_name}' , got:\n" + e.message + f"\nRefer to the algorithm subschema definition at {SCHEMA_VISUALIZATION_URL}\n" ) if algo_name in ALGO_NAME_VS_README_URL: e.message += ( f"or to the algorithm documentation for examples of the configs: " f"{ALGO_NAME_VS_README_URL[algo_name]}" ) raise e .

@LalitSP feel free to open PR, it's hard to judge by the provided code snippet.

LalitSP added a commit to LalitSP/nncf that referenced this issue Mar 23, 2024
@RitikaxShakya
Copy link

Hello! I wish to work on this issue.

@ljaljushkin
Copy link
Contributor Author

ljaljushkin commented Mar 29, 2024

Greetings, @RitikaxShakya!

Currently, @LalitSP is working on that. There's a PR with a possible fix already #2597, but it's not finished yet: some comments and tests should be resolved.
@LalitSP are you going to finish your PR? Do you need any help? Please inform us if you do not plan to continue working on this task. Thanks!

@RitikaxShakya
Copy link

@ljaljushkin Okay Cool! No problem. Thank you for replying. I would like to know if there are more gfi I can work on :)

@ljaljushkin
Copy link
Contributor Author

@ljaljushkin Okay Cool! No problem. Thank you for replying. I would like to know if there are more gfi I can work on :)

There are still contributors needed for a few GFI:
https://github.com/orgs/openvinotoolkit/projects/3

And, potentially, at least this GFI might need a new contributor - #2571

@RitikaxShakya
Copy link

@ljaljushkin Cool! Thank you so much! i will take a look, found one and will work on it! if there seems to be no activity on #2571 by current contributor then i would like to take up that issue.

@ljaljushkin
Copy link
Contributor Author

The assignee was unassigned due to the lack of activity.

@RitikaxShakya
Copy link

@ljaljushkin Can i take this issue?

@ljaljushkin
Copy link
Contributor Author

@ljaljushkin Can i take this issue?

sure! just add a comment with .take command.
https://github.com/openvinotoolkit/openvino/tree/master/?tab=readme-ov-file#take-the-issue

@ljaljushkin ljaljushkin removed their assignment Apr 3, 2024
@openvinotoolkit openvinotoolkit deleted a comment from github-actions bot Apr 3, 2024
@RitikaxShakya
Copy link

.take

Copy link

github-actions bot commented Apr 3, 2024

Thank you for looking into this issue! Please let us know if you have any questions or require any help.

@anzr299
Copy link
Contributor

anzr299 commented Apr 9, 2024

Is this issue being worked on actively? If not, I'm ready to take it up.

@RitikaxShakya
Copy link

RitikaxShakya commented Apr 9, 2024

@anzr299 Yes i am working on it.

@p-wysocki
Copy link
Contributor

Hello @RitikaxShakya, are you still working on that issue? Do you need any help?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
Status: Contributors Needed
Development

No branches or pull requests

5 participants