-
Notifications
You must be signed in to change notification settings - Fork 612
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
XFAIL failing tflite test #19123
XFAIL failing tflite test #19123
Conversation
Signed-off-by: Nirvedh <[email protected]>
815625d
to
88ea156
Compare
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.
Do you want to disable iree/tests/e2e/stablehlo_models/mnist_train_test/mnist_train_test_cpu
in this PR too?
Logs: https://github.com/iree-org/iree/actions/runs/11805834745/job/32889192224#step:7:2389
URL (same location that is no longer accessible):
MODEL_ARTIFACTS_URL = "https://storage.googleapis.com/iree-model-artifacts/mnist_train.2bec0cb356ae7c059e04624a627eb3b15b0a556cbd781bbed9f8d32e80a4311d.tar" |
# REQUIRES: llvmcpu | ||
# RUN: %PYTHON -m iree_tfl_tests.mobilenet_v1_test --target_backend=llvmcpu --artifacts_dir=%t | ||
# XFAIL: * |
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.
While XFAIL is generally useful, I would actually exclude the test here with REQUIRES
, using this pattern:
# REQUIRES: bugfix | |
# tfhub/kaggle broke their download endpoint, update URLs | |
# RUN: %PYTHON -m iree_tfl_tests.cartoon_gan_test --artifacts_dir=%t |
The specific file this test is trying to download is
model_path = "https://storage.googleapis.com/iree-model-artifacts/tflite-integration-tests/mobilenet_v1.tflite" |
That URL is returning an error. We have a tracking issue covering migrating iree-model-artifacts
GCP files to other locations: #18518 .
I'd rather keep the test disabled until we take intentional action to move the file, instead of XFAIL it and then have the test maybe start unexpectedly passing if the URL becomes live again.
#19127 will supersede this (though I didn't run the |
Sounds good. I can keep that part and refactor this PR tomorrow. |
That's fine, thanks. A better fix than just running the script as-is would be updating the tests to use files that are still available, so the test status is accurate (we should support mobilenet_v1 generally, even if our mirror of the .tflite file for mobilenet_v1 used in a test is inaccessible ;P). I have iree-org/iree-test-suites#5 for the long term solution for such tests. |
After speaking to Scott offline, I think the way the state of these tests is currently is what it should be until the long term fixes start landing so I am not going to make any changes. |
The datatset for some tests doesnt seem currently accessible. More details on
#18518