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

[FX][Conformance] Enable Conformance Test for FX Backend #3321

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

anzr299
Copy link
Collaborator

@anzr299 anzr299 commented Feb 28, 2025

Changes

  1. Save torch exported compressed model instead of OV coverted model. Save OV model which is obtained using torch.compile and dumping cache. The OV model obtained by dumping cache is used for counting FQ nodes.
  2. Save torch exported model using torch.export.save instead of converting to OV and saving OV model when saving fp32 model for debugging.
  3. Make torch compile validation default path for torch fx models.

Above changes were made to obtain the validation path:
image

Related tickets

163422

Tests

Category Job Status Job Number Notes
Manual post_training_quantization Failed 627  Expected Regression of 0.6% in Swin with FX backend
GH WC WC test on GHA Pass 36
Example Example Pass 240  

@anzr299 anzr299 requested a review from a team as a code owner February 28, 2025 19:54
@github-actions github-actions bot added the NNCF PTQ Pull requests that updates NNCF PTQ label Feb 28, 2025
@@ -94,7 +94,7 @@ def _validate(self) -> None:
predictions = np.zeros(dataset_size)
references = -1 * np.ones(dataset_size)

if self.backend in FX_BACKENDS and self.torch_compile_validation:
Copy link
Collaborator

@daniil-lyakhov daniil-lyakhov Mar 3, 2025

Choose a reason for hiding this comment

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

Please do not remove the self.torch_compile_validation option and made it True by default

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but then the default path for FX backend models will be using OV validation

Copy link
Collaborator

@daniil-lyakhov daniil-lyakhov Mar 3, 2025

Choose a reason for hiding this comment

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

Typo, my bad, I meant True by default

Comment on lines +493 to +496
if file.endswith(".bin"):
bin_file = file
elif file.endswith(".xml"):
xml_file = file
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if there are several submodels in this dir?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

submodels of the same model? or other models?
I can save in a different folder by the model name if the problem is the latter like this:
torch.compile(exported_model.module(), backend="openvino", options = {"model_caching" : True, "cache_dir": str(self.output_model_dir / self.model_name)})
instead of just "cache_dir": str(self.output_model_dir)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean one model is being cut on several part, as it was with the Yolo 11 model. As far as I remember, this means there are several IRs generated for one model which are run sequentially

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hm, but the models with graph break should not be supported right?

Copy link
Collaborator

@daniil-lyakhov daniil-lyakhov Mar 4, 2025

Choose a reason for hiding this comment

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

They should have one graph, but it is possible due to bugs in ov/nncf that there are several IRs. I wonder, what will be the result? Perhaps we shouldn't analyze the parts of the model separately

Copy link
Collaborator Author

@anzr299 anzr299 Mar 4, 2025

Choose a reason for hiding this comment

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

Then Maybe I can raise an error after a check for multiple bin and xml files in the location. The expected behavior would be to simple rename and replace the files.

@daniil-lyakhov daniil-lyakhov self-assigned this Mar 3, 2025
Copy link
Contributor

@alexsu52 alexsu52 left a comment

Choose a reason for hiding this comment

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

Could you update the description with more details? I mean, it's not entirely clear what exactly was done.

@daniil-lyakhov
Copy link
Collaborator

daniil-lyakhov commented Mar 21, 2025

@anzr299, the issue 162009 was fixed in the openvino 2025.1, please consider it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NNCF PTQ Pull requests that updates NNCF PTQ
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants