-
Notifications
You must be signed in to change notification settings - Fork 31k
fix 3 failed test cases for video_llama_3 model on Intel XPU #41931
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Liu, Kaixuan <[email protected]>
|
Thank you @kaixuanliu cc @yao-matrix for a first look 🙏 |
| distribution_name = pkg_name if pkg_name in distributions else distributions[0] | ||
| package_version = importlib.metadata.version(distribution_name) | ||
| except importlib.metadata.PackageNotFoundError: | ||
| except (importlib.metadata.PackageNotFoundError, KeyError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain what is the issue here that requires this new KeyError?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When i run pytest -rA tests/models/video_llama_3/test_modeling_video_llama_3.py::VideoLlama3ModelTest::test_generate_with_quant_cache, it will fail and return error:
pkg_name = 'optimum.quanto', return_version = True
def _is_package_available(pkg_name: str, return_version: bool = False) -> tuple[bool, str] | bool:
"""Check if `pkg_name` exist, and optionally try to get its version"""
spec = importlib.util.find_spec(pkg_name)
package_exists = spec is not None
package_version = "N/A"
if package_exists and return_version:
try:
# importlib.metadata works with the distribution package, which may be different from the import
# name (e.g. `PIL` is the import name, but `pillow` is the distribution name)
> distributions = PACKAGE_DISTRIBUTION_MAPPING[pkg_name]
E KeyError: 'optimum.quanto'
src/transformers/utils/import_utils.py:56: KeyError
Here is to fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kaixuanliu it shows 'optimum': ['optimum-quanto', 'optimum-onnx', 'optimum'], in PACKAGE_DISTRIBUTION_MAPPING, so we may need another logic to search optimum-quanto distribution in optimum package, rather than using your current logic, because the package actually there, just we find it in a wrong way in this case. @ydshieh , WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me check ,thank you for explaining the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this PR, when we catch the keyError exception, it will load optimum-quanto in package = importlib.import_module(pkg_name), where pkg_name is optimum.quanto, and it can be imported properly.
| None, | ||
| ): "user\n\nDescribe the image.\nassistant\nThe image captures a vibrant night scene in a bustling Japanese city. A woman in a striking red dress", | ||
| } | ||
| ).get_expectation() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls use format: off and format: on, and the same format as other files, refer to
| # fmt: off |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls also "follow the same format as other files", as I shared in above link, thx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have updated. Is it OK now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, thank you both!
| "user\nWhat is relativity?\nassistant\nRelativity is a scientific theory that describes the relationship between space and time. It was first proposed by", | ||
| ], | ||
| } | ||
| ).get_expectation() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
Signed-off-by: Liu, Kaixuan <[email protected]>
Signed-off-by: Liu, Kaixuan <[email protected]>
|
[For maintainers] Suggested jobs to run (before merge) run-slow: video_llama_3 |
|
@kaixuanliu @yao-matrix Thank you. I have currently some trouble to get a CI runner to check the I am happy to merge the PR if you can revert this part of change, and open an issue for this |
|
run-slow: vit, clip |
|
This comment contains models: ["models/clip", "models/vit"] |
CI Results✅ No failing test specific to this PR 🎉 ! |
This PR fixes 3 failed test cases on Intel XPU: