fix analyze coverage bugs#922
Conversation
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Review of PR #922 (fix analyze coverage bugs). Three separate bug fixes are bundled here. The
ode_stable_key fix and return-tuple reorder are correct. The
esolve_rule_parquet_path refactor introduces regressions described in inline comments.
- Guard `ref_device = devices[0]` against an empty device list so a programmatic `device=None` exits cleanly (exit 2) instead of raising an unguarded IndexError that bypasses the command-wide error handling. - For `ep=auto` + `device=all`, resolve the best available EP per device rather than picking one EP from a single ref device and fanning it across unrelated devices. Add an auto-all case to test_selection_matrix.
…anup Address PR #922 review feedback: - WINMLCLI_RULES_DIR / WINMLCLI_RULES_DIR_FOR_DEBUG now resolve a single directory and are no longer split on os.pathsep. This removes the half-supported multi-dir behavior where only the first listed directory was ever probed, making rule lookup deterministic with no surprising fallback. - Update docstrings, the rules README, and the CHANGELOG migration note to match the single-directory semantics; refresh tests accordingly. - In _load_parquet_rule_table, drop the resolved_parquet_path temporary and check parquet_path.exists() only after the cache-hit early return (avoids a filesystem probe on every cache hit). Document that the per-instance cache assumes the rule-dir env vars are stable for the query's lifetime.
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Follow-up review of PR #922. Both previously flagged bugs in resolve_rule_parquet_path are addressed — Bug 1 via an explicit single-directory design, Bug 2 via an intentional semantic change (documented and tested). The cache warning is addressed with an explanatory comment. One minor note inline.
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
All previously flagged bugs are addressed. Bug 1 (multi-dir search) is resolved by making single-directory support explicit — _get_env_rules_dir no longer splits on os.pathsep, and the README/CHANGELOG are updated. Bug 2 (for_debug fallback removal) is an intentional semantic change: when WINMLCLI_RULES_DIR_FOR_DEBUG is set, lookup returns the debug-dir candidate exclusively; this is documented, tested, and explained in the commit message. The cache warning is addressed with a comment and the parquet_path.exists() probe is correctly moved after the cache-hit return. The
ode_stable_key fix and tuple-reorder fix are correct and well-tested. LGTM.
2 participants
No description provided.