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

f32 precision for compare-with-transformers tests #508

Merged
merged 3 commits into from
Jan 10, 2024
Merged

Conversation

helena-intel
Copy link
Collaborator

Allow these tests to pass locally on devices where inference is run by default in FP16 (GPU) or BF16 (latest Xeon).

Also added an OpenVINO version check to make a test pass in 2023.3 (get_property now returns a string instead of an object).

Allow these tests to pass locally on devices where inference is run by default in FP16 (GPU) or BF16 (latest Xeon).
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@@ -430,7 +440,9 @@ class OVModelForFeatureExtractionIntegrationTest(unittest.TestCase):
def test_compare_to_transformers(self, model_arch):
model_id = MODEL_NAMES[model_arch]
set_seed(SEED)
ov_model = OVModelForFeatureExtraction.from_pretrained(model_id, export=True)
ov_model = OVModelForFeatureExtraction.from_pretrained(
model_id, export=True, ov_config={"CACHE_DIR": "", "INFERENCE_PRECISION_HINT": "f32"}
Copy link

Choose a reason for hiding this comment

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

I suggest using OV_CONFIG const in utils and loading it via:
ov_model = OVModelForFeatureExtraction.from_pretrained(
model_id, export=True, ov_config=OV_CONFIG)

Copy link

Choose a reason for hiding this comment

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

also setting NUN_STREAMS = 1 could be useful

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suggest using OV_CONFIG const in utils and loading it via:
ov_model = OVModelForFeatureExtraction.from_pretrained(
model_id, export=True, ov_config=OV_CONFIG)

I like being explicit in tests, especially when we deviate from defaults. But I can add an F32_CONFIG at the top of this file.

also setting NUN_STREAMS = 1 could be useful

Agreed that this can be useful, but we should run some tests and assuming no issues, set this by default in optimum-intel, not just for the tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like being explicit in tests, especially when we deviate from defaults. But I can add an F32_CONFIG at the top of this file.

Done

Choose a reason for hiding this comment

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

the same will be needed also in stable diffusion tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the same will be needed also in stable diffusion tests

Thanks @dtrawins I did that in this PR #518

Copy link
Collaborator

@echarlaix echarlaix 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 addition @helena-intel

@@ -125,7 +128,10 @@ def test_load_from_hub_and_save_model(self):
loaded_model = OVModelForSequenceClassification.from_pretrained(self.OV_MODEL_ID, ov_config=ov_config)
self.assertTrue(manual_openvino_cache_dir.is_dir())
self.assertGreaterEqual(len(list(manual_openvino_cache_dir.glob("*.blob"))), 1)
self.assertEqual(loaded_model.request.get_property("PERFORMANCE_HINT").name, "THROUGHPUT")
if get_version() < "2023.3":
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor comment : it could be replaced with is_openvino_version for clarity

Suggested change
if get_version() < "2023.3":
if is_openvino_version("<", "2023.3"):

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Changed.

@echarlaix
Copy link
Collaborator

Thanks a lot @helena-intel

@echarlaix echarlaix merged commit aa5b71b into main Jan 10, 2024
12 checks passed
@echarlaix echarlaix deleted the helena/tests-f32 branch January 10, 2024 10:30
PenghuiCheng pushed a commit to PenghuiCheng/optimum-intel that referenced this pull request Jan 16, 2024
* f32 precision for compare-with-transformers tests

Allow these tests to pass locally on devices where inference is run by default in FP16 (GPU) or BF16 (latest Xeon).

* Add F32_CONFIG constant for modeling tests

* Replace get_version with is_openvino_version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants