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

#2570 Issue: Update config.py #2597

Closed
wants to merge 3 commits into from

Conversation

LalitSP
Copy link

@LalitSP LalitSP commented Mar 23, 2024

Changes

Updated the validation logic in the config.py file to handle errors related to additional properties not defined in the schema. Added checks to raise a more informative ValidationError when unexpected properties are encountered during validation.

Reason for changes

The current error reporting mechanism in the JSON schema validation is misleading, especially when encountering unexpected properties in the configuration.

Related tickets

Ticket ID: 94933

Tests

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

@LalitSP LalitSP requested a review from a team as a code owner March 23, 2024 08:02
@github-actions github-actions bot added the NNCF PT Pull requests that updates NNCF PyTorch label Mar 23, 2024
Copy link

codecov bot commented Mar 23, 2024

Codecov Report

Attention: Patch coverage is 16.66667% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 83.56%. Comparing base (b7ba5ad) to head (9621ee4).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2597      +/-   ##
===========================================
- Coverage    91.16%   83.56%   -7.61%     
===========================================
  Files          494      494              
  Lines        45350    45374      +24     
===========================================
- Hits         41342    37915    -3427     
- Misses        4008     7459    +3451     
Files Coverage Δ
nncf/config/config.py 76.31% <16.66%> (-3.97%) ⬇️

... and 81 files with indirect coverage changes

Flag Coverage Δ
COMMON ?
ONNX 34.66% <16.66%> (+0.01%) ⬆️
OPENVINO ?
TENSORFLOW 30.10% <16.66%> (-0.02%) ⬇️
TORCH 65.92% <16.66%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
common 88.89% <ø> (-4.90%) ⬇️
torch 93.48% <ø> (ø)
tensorflow 93.74% <ø> (ø)
onnx 93.12% <ø> (+0.09%) ⬆️
openvino 25.75% <ø> (-68.39%) ⬇️
ptq 64.81% <ø> (-25.31%) ⬇️

@alexsu52 alexsu52 requested a review from ljaljushkin March 24, 2024 09:15
Copy link
Contributor

@ljaljushkin ljaljushkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! Please address the comment and fix 2 precommit jobs: common unit tests and type checks.

@@ -127,13 +128,17 @@ def _is_path_to_algorithm_name(path_parts: List[str]) -> bool:
def validate(loaded_json):
try:
jsonschema.validate(loaded_json, NNCFConfig.schema())

except JsonSchemaValidationError as e:
logger.error('Invalid NNCF config supplied!')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need this error handler? Isn't it the same as the next one? Shouldn't the next one executed?

@ljaljushkin
Copy link
Contributor

#2570

@ljaljushkin
Copy link
Contributor

Due to the lack of activity @LalitSP was unassigned from the issue and PR is closed

@ljaljushkin ljaljushkin closed this Apr 3, 2024
@LalitSP LalitSP deleted the LalitSP/issue2570 branch April 8, 2024 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NNCF PT Pull requests that updates NNCF PyTorch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants