Enhance and optimize the model ops test generation and script for updating failures reason for the failed models ops test #1198
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
||||||||||||||
1 similar comment
|
||||||||||||||
|
||||||||||||||
1 similar comment
|
||||||||||||||
chandrasekaranpradeep
force-pushed
the
pchandrasekaran/enhance_models_ops_test_gen
branch
from
7474899 to
3ccbbd1
Compare
chandrasekaranpradeep
requested review from
nvukobratTT,
pilkicTT and
dgolubovicTT
as code owners
|
||||||||||||||
1 similar comment
|
||||||||||||||
|
||||||||||||||
1 similar comment
|
||||||||||||||
nvukobratTT
reviewed
Feb 7, 2025
forge/forge/python_codegen.py
Outdated
| self.indent -= 1 | ||
| self.wl("") | ||
| if "pcc" not in exclude_record_property: | ||
| self.wl("pcc = 0.99") | ||
| if "integer_tensor_high_value" not in exclude_record_property: |
There was a problem hiding this comment.
Let's think about having these hard-codded configurations as part of the separated configuration file. E.g. specific config object for this tool.
I assume that we'll have more of this kind, so it might be good to think about how to manage. Also, as this PR is quite big, let's create issues for this and tackle it as separate PR.
There was a problem hiding this comment.
Okay Nikola.
Can you give some idea on config object functionalities like whether it is should be used inside the generated models ops test or it is utility used by the write_pytest_method method in ForgeWritter?
| @@ -2784,7 +2762,23 @@ def delete_unneeded_outputs(ops, returns): | |||
| # Generate unique op tests based on requested model. Currently only supported | |||
| # for PyTorch framework. | |||
| if compiler_cfg.extract_tvm_unique_ops_config or compiler_cfg.tvm_generate_unique_ops_tests: | |||
|
|
|||
| # Commenting the below verification between framework outputs and generated forge module outputs | |||
There was a problem hiding this comment.
Hmm, the majority of them are failing during framework vs codegen?
@dgolubovicTT something to think about during initial support for verifications after different compile stages. Maybe we should review TVM side of verification as well during this push, before pushing next stage of intermediate verification.
There was a problem hiding this comment.
FYI, I have tried generating models ops test in latest main for the below 20 models, out of the 20 models, 2 models test cases are failing with framework vs codegen verification
Failed models test:
forge/test/models/pytorch/text/codegen/test_codegen.py::test_codegen[Salesforce/codegen-350M-mono]
forge/test/models/pytorch/text/gptneo/test_gptneo.py::test_gptneo_causal_lm[EleutherAI/gpt-neo-125M]
Note: I haven't triggered the models ops test generation pipeline for all the models in the forge, I have test for sample of 20 text based models for checking the behaviour of pipeline.
| @@ -1011,6 +1033,27 @@ def generate_models_ops_test(unique_operations: UniqueOperations, models_ops_tes | |||
| pytest_input_shapes_dtypes.append((operand_shape, operand_dtype)) | |||
| pytest_input_shapes_and_dtypes_list.append(pytest_input_shapes_dtypes) | |||
|
|
|||
| # To avoid recording pcc in record_property pytest fixture and add the pcc to the exclude metadata property list | |||
| exclude_record_property = ["pcc"] | |||
There was a problem hiding this comment.
Also a candidate for tooling configuration object.
There was a problem hiding this comment.
Will include it.
| if op_name == "embedding": | ||
| # Calculate embedding op indicies tensor maximum value based upon the num_embeddings of the weight tensor. | ||
| pytest_metadata["integer_tensor_high_value"] = int(operand_shapes[1][0]) - 1 | ||
| exclude_record_property.append("integer_tensor_high_value") |
There was a problem hiding this comment.
This can probably be generally excluded as part of the configuration object.
There was a problem hiding this comment.
Okay.
nvukobratTT
approved these changes
Feb 7, 2025
chandrasekaranpradeep
force-pushed
the
pchandrasekaran/enhance_models_ops_test_gen
branch
from
3ccbbd1 to
9c85e4f
Compare
ashokkumarkannan1
approved these changes
Feb 10, 2025
|
||||||||||||||
1 similar comment
|
||||||||||||||
|
||||||||||||||
1 similar comment
|
||||||||||||||
chandrasekaranpradeep
force-pushed
the
pchandrasekaran/enhance_models_ops_test_gen
branch
from
9c85e4f to
2c4932e
Compare
|
||||||||||||||
1 similar comment
|
||||||||||||||
|
||||||||||||||
1 similar comment
|
||||||||||||||
|
@nvukobratTT FYI,
|
chandrasekaranpradeep
force-pushed
the
pchandrasekaran/enhance_models_ops_test_gen
branch
from
2c4932e to
e1f0e2c
Compare
…ating failures reason for the failed models ops test
chandrasekaranpradeep
force-pushed
the
pchandrasekaran/enhance_models_ops_test_gen
branch
from
e1f0e2c to
aa85b55
Compare
|
||||||||||||||
|
||||||||||||||
1 similar comment
|
||||||||||||||
|
||||||||||||||
chandrasekaranpradeep
deleted the
pchandrasekaran/enhance_models_ops_test_gen
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1122
Removed the dependency of pt files in Model ops test generation pipeline which resolve space issue in CI and local machines by avoid storing pt files.
Fixes #1199
Created a script for updating the model ops test failures with xfail marker and failures reason by using pytest logs files which is saved as artifacts in CI
Fixes #795
Enabled the option for recording properties such as frontend, op_name, model_name and op_params
TODO:
Need to update other record properties, will work on adding it in separate PR.
Attached the generated model ops test folder before and after the model ops test failure update script.
models_ops_before_xfail_update.zip
models_ops_after_xfail_update.zip
Note:
Added the verification between framework outputs and generated forge module outputs before extract_and_generate_unique_ops_tests function in forge/forge/tvm_to_python.py which is used for extracting the unique ops configuration and generated unique ops test but in the current commit lots of models are failling in the verification so need to check on the latest main.