-
Notifications
You must be signed in to change notification settings - Fork 4
fix: op-tracing preflight, eval samples merge, compile device label, HTP tagged_nodes stats #209
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
Closed
Closed
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
ffa80c1
bug fix
847bcce
fix: address PR review comments on bugs A-D and eval/dataset refactor
f9e4cf4
Merge branch 'main' into hualxie/fix_bugs
xieofxie 355be3b
Merge branch 'main' into hualxie/fix_bugs
xieofxie 2d24a00
Merge branch 'main' into hualxie/fix_bugs
xieofxie File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| # ------------------------------------------------------------------------- | ||
| # Copyright (c) Microsoft Corporation. All rights reserved. | ||
| # Licensed under the MIT License. | ||
| # -------------------------------------------------------------------------- | ||
| """Tests for HTPExporter export statistics correctness.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from unittest.mock import MagicMock | ||
|
|
||
| from winml.modelkit.export.htp import HTPExporter | ||
|
|
||
|
xieofxie marked this conversation as resolved.
|
||
|
|
||
| class TestHTPExporterTaggedNodesStats: | ||
| """Bug D: tagged_nodes and coverage must be 0 when embed_hierarchy_attributes=False.""" | ||
|
|
||
| def test_tagged_nodes_zero_when_hierarchy_disabled(self) -> None: | ||
| exporter = HTPExporter(embed_hierarchy_attributes=False) | ||
| exporter._node_tagger = MagicMock() | ||
| exporter._node_tagger.tag_all_nodes.return_value = { | ||
| "node1": "/Model/Layer1", | ||
| "node2": "/Model/Layer2", | ||
| "node3": "/Model/Layer3", | ||
| } | ||
| exporter._node_tagger.get_tagging_statistics.return_value = {} | ||
|
|
||
| mock_model = MagicMock() | ||
| mock_model.graph.node = [MagicMock() for _ in range(5)] | ||
|
|
||
| exporter._apply_hierarchy_tags(mock_model) | ||
|
|
||
| assert exporter._export_stats["tagged_nodes"] == 0 | ||
|
|
||
| def test_coverage_zero_when_hierarchy_disabled(self) -> None: | ||
| exporter = HTPExporter(embed_hierarchy_attributes=False) | ||
| exporter._node_tagger = MagicMock() | ||
| exporter._node_tagger.tag_all_nodes.return_value = { | ||
| "n1": "/t1", | ||
| "n2": "/t2", | ||
| } | ||
| exporter._node_tagger.get_tagging_statistics.return_value = {} | ||
|
|
||
| mock_model = MagicMock() | ||
| mock_model.graph.node = [MagicMock() for _ in range(4)] | ||
|
|
||
| exporter._apply_hierarchy_tags(mock_model) | ||
|
|
||
| assert exporter._export_stats["coverage_percentage"] == 0.0 | ||
|
|
||
| def test_tagged_nodes_nonzero_when_hierarchy_enabled(self) -> None: | ||
| """Control: stats are populated normally when embedding is enabled.""" | ||
| exporter = HTPExporter(embed_hierarchy_attributes=True) | ||
| exporter._node_tagger = MagicMock() | ||
| exporter._node_tagger.tag_all_nodes.return_value = { | ||
| "n1": "/t1", | ||
| "n2": "/t2", | ||
| } | ||
| exporter._node_tagger.get_tagging_statistics.return_value = {} | ||
|
|
||
| mock_model = MagicMock() | ||
| mock_model.graph.node = [MagicMock() for _ in range(4)] | ||
|
|
||
| exporter._apply_hierarchy_tags(mock_model) | ||
|
|
||
| assert exporter._export_stats["tagged_nodes"] == 2 | ||
| assert exporter._export_stats["coverage_percentage"] == 50.0 | ||
Oops, something went wrong.
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.
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.
I dont quite get the default value changes here, why remove default values from cli args, and delay to the function call?
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.
I have updated the previous bugbash report in description
Bug B — P2: wmk eval --samples N silently ignored when no --dataset is given
Command: wmk eval -m microsoft/resnet-50 --samples 20
Symptom: Output always shows 'samples': 100 regardless of --samples value.
Root cause: evaluate.py:149-155 — when config.dataset.path is None (no --dataset flag), the entire DatasetConfig is replaced wholesale with the hardcoded _DEFAULT_DATASETS entry (which has samples=100), discarding the user's value:
if config.dataset.path is None:
config.dataset = deepcopy(default) # overwrites samples, split, shuffle
Fix: Merge user-specified fields (samples, split, shuffle, seed) onto the default rather than replacing it entirely.
Location: src/winml/modelkit/eval/evaluate.py:155
The None could let user decide either user value or default config value could take effect