Repository navigation
ALMIOPEN-2502: harden winner cache follow-ups - #11804
Merged
Merged
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #11804 +/- ##
===========================================
- Coverage 70.54% 69.42% -1.13%
===========================================
Files 2810 2828 +18
Lines 462436 470102 +7666
Branches 68077 69415 +1338
===========================================
+ Hits 326219 326332 +113
- Misses 112673 120069 +7396
- Partials 23544 23701 +157
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
SamuelReeder
added
the
ci:host-asan
- Restore the String-vector branch's closing brace, mistakenly absorbed by the new Float/Double elif (BLOCKING-1, confirmed by external review compiling the emitted header). - Add TestEmittedBodiesAreWellFormed: assert brace balance per vector element kind, so a dropped brace fails a test instead of only an assertIn on body text (BLOCKING-2, the regression test for the above). - Reorder cache_uid_domain validation so the duplicate-annotation check (declared on N fields) runs before the candidate-ambiguity check; the natural mistake now gets the more specific diagnosis (IMPORTANT-5). - Stop wrapping to the last schema object on a missing element index in the domain-candidate/resolution paths; skip or raise instead (SUGGESTION-6). - Tighten the float/double memcmp tests to assert "!= 0" is part of the emitted condition, closing a != -> > mutation gap. - Give ContentCarryingTestGraph a required (non-optional) TensorValue case, so the non-optional scalar Float memcmp site gets real byte-level coverage instead of 0.
…OCm/rocm-libraries into users/sareeder/almiopen-2502-followups
- sortedCatalog() now mirrors catalogFor()'s own reject guard before computing a cache key. cacheKey() reads only graph id and device ordinal, never arch, so an unresolved-arch context previously cached its rejected, empty catalog under the SAME key a later, resolved call for that device reuses -- permanently hiding the real catalog. Reproduced fail-before/pass-after against the extended AnEmptyArchNameYieldsAnEmptyCatalog test. - PluginDevelopment.md:419 overclaimed that a primed benchmarking plan "re-samples every candidate on each execute"; BenchmarkPlan resolves once per plan lifetime (TheWinnerIsResolvedOnceAcrossRepeatedExecuteCalls covers this). Narrowed the claim to the priming execute specifically.
Contributor
TheRock Submodule Bump ActivityNewest first
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Harden the in-process winner cache follow-ups from the ALMIOPEN-2502 review. Avoid graph-key construction on the cold no-op path, reject unidentified devices, verify retained graph buffers, and keep generated cache-key hashing and equality consistent for floating-point values.
JIRA ID : ALMIOPEN-2502
Risk Assessment
Risk level 3. The change touches winner-cache lookup, graph-key validation, and generated cache-key equality in the plugin and FlatBuffers SDKs. The behavior is covered by focused unit tests, fail-before/pass-after evidence, and the full affected SDK test suites. It does not add a provider operation or architecture-specific kernel path.
KernelIngestorStateManager::mightHaveWinnerFormoved from private to public soGenericPlanBuildercan call it; it is header-only and templated, so this is not an ABI change, but it is a new public surface. A second review pass found a real correctness defect this PR's own tests did not cover (a rejected empty-architecture catalog could be cached and served for a later, resolved device); it is fixed and now has fail-before/pass-after regression coverage.ASIC Coverage
The change is architecture-independent host-side cache and validation logic. It does not change kernel support, dispatch selection for a valid device, or provider registration. Standard PR CI and host ASAN coverage are sufficient; no specific-ASIC run or full multi-arch sweep is required.
Testing Summary
developand after the second-pass catalog-cache fix.HIPDNN_ENABLE_KERNEL_INGESTOR=ON,ENABLE_CLANG_FORMAT=ON, andENABLE_CLANG_TIDY=ON.ci:host-asanlabel; the workflow is pending.Fail-Before / Pass-After Evidence
The baseline runs used a temporary tree with the new tests retained and the affected product files restored from
origin/develop.cd projects/hipdnn/scripts && python3 -m unittest test_gen_cache_key.TestPortableByteOperations.test_a_float_field_compares_its_object_representation test_gen_cache_key.TestPortableByteOperations.test_a_float_vector_compares_elements_by_object_representation test_gen_cache_key.TestUidCanonicalization.test_a_second_domain_shaped_candidate_is_rejected test_gen_cache_key.TestUidCanonicalization.test_more_than_one_domain_annotation_is_rejected -vproduced 3 failures (scalar float comparison, float-vector comparison, and second domain-shaped candidate); the duplicate annotation test passed. The same four tests pass on this tree.hipdnn_flatbuffers_sdk_tests --gtest_filter="TestGraphContentKey.TwoGraphsCarryingTheSameNanFloatComparePairwiseEqual:TestGraphContentKey.TwoGraphsCarryingDifferentNanPayloadsCompareUnequal:TestGraphContentKey.AGraphWithMalformedBytesYieldsAnUnusableKey"failed the identical-NaN test and then crashed withSegmentation faultin the malformed-buffer test. The patched command passes all 3 tests.hipdnn_plugin_sdk_tests --gtest_filter="TestIngestorWinnerCacheStateManager.AnEmptyArchNameYieldsAnEmptyCatalog"failed withActual: false, Expected: true; the patched command passes.sortedCatalog()cached a rejected, empty catalog under a key ({graph, device ordinal}) that ignores architecture; the extendedAnEmptyArchNameYieldsAnEmptyCatalog(restoring the architecture after the empty-arch call) failed withActual: true, Expected: falseagainst the code before this fix; the patched command passes.hipdnn_plugin_sdk_tests --gtest_filter="TestIngestorGenericPlanBuilder.ASecondColdMissWithBenchmarkingOffDoesNotReadGraphBytes"reportedbytesCalls->load() Which is: 1, while the patched command reports 0 and passes.Testing Checklist
python3 -m unittest discover -s projects/hipdnn/scripts -p "test_gen_cache_key.py" -v- Status: Passed (43 tests)ctest --test-dir build/projects/hipdnn --output-on-failure -R '^(hipdnn_plugin_sdk_tests|hipdnn_flatbuffers_sdk_tests|hipdnn_cache_key_codegen_tests)$'- Status: Passed (3/3)hipdnn_flatbuffers_sdk_tests --gtest_filter='*Malformed*:*Nan*',hipdnn_plugin_sdk_tests --gtest_filter='*AnEmptyArchNameYieldsAnEmptyCatalog', andhipdnn_plugin_sdk_tests --gtest_filter='*ASecondColdMissWithBenchmarkingOffDoesNotReadGraphBytes'- Status: Passedcmake --fresh --preset hipdnn -B build -DHIPDNN_ENABLE_KERNEL_INGESTOR=ONandcmake --build build- Status: Passedpre-commit run --files $(git diff --name-only origin/develop...HEAD)- Status: PassedTechnical Changes
WinnerKeyconstruction with the cheap disk-aware winner probe and reject empty device architecture names.BenchmarkPlanprivate state.std::memcpy, bitwise float/double equality, and bitwise float/double vector equality; reject ambiguous domain-shaped cache-key vectors.GetRoot.IGraphvirtual-order comments.ifin emitted code that only compiles today because no schema reachable fromGraphhas a string-element vector); add a test asserting brace balance across every vector element kind so the defect fails a test rather than only anassertInon body text.cache_uid_domain-annotation check before the candidate-ambiguity check, so the more specific diagnosis fires for the natural mistake of annotating two domain-shaped fields.memcmpgenerator tests to require!= 0in the emitted condition, and give the C++ graph-content-key test fixture a required (non-optional)TensorValuecase so that scalar-Float comparison site gets real byte-level coverage.sortedCatalog()with the same reject condition ascatalogFor(), so a context with an unresolved device or empty architecture never writes its rejected, empty catalog into_catalogCacheunder the same(graph, device ordinal)key a later, resolved call for that device would reuse.docs/PluginDevelopment.md's claim that a primed benchmarking plan "re-samples every candidate on each execute";BenchmarkPlanresolves once per plan lifetime.