Qualcomm AI Engine Direct - [GenAI Pipeline Phase 2] PRB1 - Compilation foundation - #22846
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22846
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 6748db8 with merge base e5acd53 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@pytorchbot label "release notes: qualcomm" |
b547d15 to
dcc47e3
Compare
|
@claude Review this pr , and share a high level and technical summary on how this improves underlying things and sets up foundation |
|
Claude finished @psiddh's task in 2m 2s —— View job PR review: GenAI Pipeline Phase 2, PRB1 (compilation foundation)
High-level summaryPhase 1 built the pipeline's structure (stages, strategies, adapters, typed configs), but compilation was a stub: How it improves things:
This leaves Stream B ready for PR-B2 (multi-graph lowering, weight sharing, sharding, spill-fill). The Technical walkthrough
The layering is sound. The adapter compiles exactly one graph. Grouping several graphs into one Review findings1. Once real specs are passed, the pipeline would always fail the target check (the SoC is a string, not the enum). 2. 3. 4. 5. Docstring nits.
6. CI. The one failure, VerdictThe foundation looks solid: clear naming, a reasonable decision on what is frozen, and the two bug fixes are real. I'd fix #1 before PR-B2 connects real compile specs to the adapter, and #2 before any CLI uses I didn't run the test suite: the sandbox didn't approve Python commands, so this review is from reading the code. Findings #1 and #2 are confirmed by tracing the call sites and the |
dcc47e3 to
6748db8
Compare
|
Regarding Claude's comments:
@psiddh Kindly take a look when you get chance, I think this PR should be GTG now. |
Summary
First PR of Phase 2 and the start of Stream B (compilation). Phase 1 added the skeleton, interfaces, stage wrappers, typed configs with stubbed logic. This PR adds the compilation groundwork the rest of Stream B builds on and makes
DefaultCompilerAdapteractually compile a graph to a.pte.Nothing under
examples/changes. The legacyllama.pyflow is untouched and stays CI's reference; the two coexist until Phase 3, whose deletion of the legacy path is gated on a parity test added in PR-A2.What's included
Two separate name spaces:
artifact_keys.py,graph_names.pyAn artifact key names a
.ptefile; a graph name names a method inside one. A hybrid decoder produces one.pte(text_decoder) holding two methods (kv_forward,prefill_forward), so these are deliberately distinct key spaces.pte_paths, so a compiled bundle reaches inference without translation. PlusDECODE_QDQ_FILENAME, a filename (a.pt2the SQNR eval reads, never seen by the runner) and therefore deliberately absent fromALL_ARTIFACT_KEYS.DECODER_GRAPH_NAMESis decode first, and the order matters:kvmode builds no prefill graph, and decode is the authoritative source of the artifact's constant methods.examples/.../decoder_constants.pyrather than importing them, so this package does not depend on the example scripts. Agreement tests keep them in step; the legacy copies go away with the legacy flow.graph_bundle.py:GraphBundle, the frozen per-graph unit quantization hands to compilation (module,inputs,meta,quant_io_dtypes,modality_inputs,executorch_config){graph_name: GraphBundle}map rather than several parallel dicts that can drift apart.frozen=Truestops in-place edits across a stage boundary. Immutability stops at the field boundary though,convert_pt2emutatesmetain place and the docstring says so, so the guarantee is not overread.quant_io_dtypesis the one thing compilation cannot derive: the graph-boundary dtypes follow from the quantization recipe's bit widths.Nonemeans quantization was skipped.__post_init__requires both keys or neither, because the tagger indexes both unconditionally so a partial mapping is aKeyErrorat lowering, not a partially-quantized boundary.CompilationInputConfig.graphsis the consumer side, added here; PR-A1 adds the producer side, PR-B2 is the first stage to read it. Defined here so both streams import one definition.compilation/compile_spec_builder.py:QnnCompileSpecBuilder,resolve_soc_model,resolve_backend_typegenerate_htp_compiler_spec/generate_gpu_compiler_specwithgenerate_qnn_executorch_compiler_specbehind onebuild();llama.pyrepeats that branch three times today.resolve_soc_modelconvertsstr→QcomChipset.PipelineContextcarries the SoC as a string so nothing in the pipeline imports the QNN schema; lowering needs the enum. Per earlier review this belongs in the adapter layer.resolve_backend_typeuses a lookup map built from the enum itself, sinceQnnExecuTorchBackendType.__str__already yields the CLI name so no hardcoded table, and a bad name raisesValueErrorlisting the valid ones.enable_x86_64is constructor state, not a per-call flag: the emulator supports neither weight sharing nor shared buffers, which is exactly whyllama.pywritesnot args.enable_x86_64at all three call sites. Hardcoding either on would break the x86 CI run without failing anything locally.shared_bufferis on, whileControlArgs.shared_bufferdefaults off to mirrorllama.py's parser. A caller driving the builder from aControlArgsmust pass it explicitly. Documented onbuild()and pinned by two tests.control_args.py: typed bridge fromPipelineContextto the existing LLM componentsargparse.Namespace(args.max_seq_len,args.enable_x86_64, …), so this is a dataclass carrying the same 65 attributes, buildable from aPipelineContext.argparse.Namespace, and that is load-bearing:QnnConfig.load_configdispatches onisinstance(config, argparse.Namespace), so a plain dataclass would break the device path.build_parser()derives anargparseparser from the same fields, so a CLI entry point stops maintaining a second default table.llama.pyparser by a test, since this is a second configuration surface and drift would be silent. Two fields are explicitly exempt (train_config,lr_configdefault toNonerather than to YAML paths underexamples/), with a test pinning that the exemption is deliberate.artifact_paths:List[Path]→Dict[str, Path](CompilationResult,CompilationOutputConfig,InferenceInputConfig,DeviceRunnerAdapter,DefaultDeviceRunnerAdapter)Keyed because the runner addresses artifacts individually and which ones exist varies by model, so position carries no reliable meaning. Absent artifacts are omitted, never mapped to
Noneso the runner tests key membership to decide whether a model is multimodal. This is the frozen B→A2 interface.DefaultCompilerAdapterimplementation: replaces PR6'sNotImplementedErrorMemoryPlanningPass(alloc_graph_input=False, alloc_graph_output=False)+BuildQuantIo(), since with a shared buffer the caller supplies graph I/O addresses from RPC memory), writes the.pte, and returns it under theartifact_keythe caller names keyed by the caller because a 1:1 adapter cannot tell a vision encoder from a text decoder when both arrive asmodel.soc_modelandbackend_typeare validated againstcompile_specsrather than trusted. Lowering takes its target only from the specs, so these parameters would otherwise be decorative: a caller could validate ops for one SoC and compile for another and see nothing until the artifact ran on device. The target is read back out of the specs withflatbuffer_to_optionand a disagreement raises. Specs with no QNN entry skip the check..pteis the strategy's job, so the adapter stays a 1:1 wrapper) and spill-fill sizing (a property of the group, computed from the lowered program). Both land in PR-B2.Two bugs found along the way (worth a look, not specific to this feature)
DefaultDeviceRunnerAdapter.push_artifactsdid[str(p) for p in artifact_paths]. Once that is a dict, iteration yields keys, so it would have pushed the strings"text_decoder","tok_embedding"… instead of paths. Now.values(), matchingEvalBase._get_adb. This is why the type change could not be a pure annotation edit.CompilationInputConfigimportedCompileSpecfromexecutorch.exir.backend.compile_spec, which does not exist (it iscompile_spec_schema). Hidden at runtime byTYPE_CHECKING; mypy flags it.Tests: 6 new files, 8 updated. Beyond the unit tests,
DefaultCompilerAdapterwas run against the real stack (a small module compiled through QNN to a non-empty.pteon the x86 path); that takes ~15s so the committed adapter tests mock lowering, while the builder tests keep two real-API cases (specs really areCompileSpecs;use_multi_contexts+online_prepareis rejected rather than masked).PR Review Checklist
examples/files touched;llama.pyand the wrappers import unchanged).Related PRs
Phase 2 flow.
PR-A1andPR-B1are independent and can land in either order.PR-B2depends onPR-B1.PR-A2depends on all three, and closes Phase 2 by wiring the runner and adding the parity test that gates the Phase-3 deletion of the legacy flow.Phase 1 (merged):
Phase 2:
model_lookup, CLI,LLMQuantizerAdapter, multi-graph quantization & encoding reconciliation. ConsumesControlArgs,GraphBundleandgraph_namesfrom this PR, but does not depend on it landing first; pendingllama_stories_260kE2E parity test. Depends on PR-A1 and PR-B2; pendingTest plan
Run only tests added in this PR:
Result:
Run all
genai_pipelinetests:Result:
Run all tests with coverage:
Result:
Confirm the legacy flow is unaffected:
Result: