Repository navigation
refactor(orchestrator): pass actions, state and trace-analysis inputs as typed objects instead of parallel keywords - #1972
Conversation
lishuoshuo-amd
added
the
size-exception
Code metrics gate: PASSEDJudged: the 87 files this change touches. Files it does not touch never fail it; anything found there is listed under Outside this change for information.
Module length warning: over 800 lines, within the 1200-line failure limit (not failing): 10
Already over at the base (backlog, not this change's): 9
87 findings in files this change does not touchOutside this change (informational): 87
...and 62 more; the job log lists every row.
Thresholds and sources: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| from ..bus.message_bus import Message | ||
| from hyperloom.inference_optimizer.trace.conversation_trace import ConversationRecord, append_conversation | ||
| from ..state.failure_evidence import UNMEASURED_OUTCOMES, render_failure_line | ||
| from ..state.shared_state import KbInjection |
|
|
||
| from ..collaborator import CoordinatorCollaborator | ||
| from ..measurement.runtime_findings import render_runtime_findings | ||
| from ..state.shared_state import KbInjection |
6489142 to
03366d3
Compare
PR #1972 -- refactor(orchestrator): pass actions, state and trace-analysis inputs as typed objects instead of parallel keywordsWhat it does: Functions in Blocking issues: none V4 -- not reported: advisory (#1952 shares 9 files, #1762 shares 6, #1867 shares 5; the body names only #1970) Step 7 independent reader: a second reader re-ran the Python 3.10/3.11 imports of round_store and the changed modules, test_framework_gap_composer.py, and the scope count (95 over in the three directories, 13 in untouched files, 66 done). Checked: all 42 changed production files, including the Tier 1
LGTM |
…idence of an attempt
…objects, read the roofline snapshot from last_trace_analyze
…DK entry points as objects
…ts, orchestrator outcome and split job as objects
…ontext, size a roofline from one KernelAggregate
…aceAnalysisOptions in analyze_trace
…the manifest from CaptureShard records
… count as one WorkloadTotals
…ort CoverageMeasures beside it
…om the microbenchmark path
…iptable runs from typed records
…p gates and log baseline
…nch from one ActorLaunch
…s take a _KbLesson, keep paths read the attempt
…s attempt with _AnalysisAttempt
…d derive the tool from the route
…intReport and time a site with one _SiteTiming
…op the never-passed rejected_hunks
… a patch(1) variant with one _PatchMode
…lysis_md already derived
… place and read launch geometry off the buffers
…st requests from one helper
…Python 3.10/3.11 accept the dataclass
…nch geometry without re-checking them
03366d3 to
3e4596d
Compare
PR #1972 -- refactor(orchestrator): pass actions, state and trace-analysis inputs as typed objects instead of parallel keywordsWhat it does: Functions in Blocking issues: none V4 -- not reported: advisory (#1952 shares 9 files, #1762 shares 6, #1867 shares 5; the body names #1970, #1971 and #1973 only) Step 7 independent reader: a second reader re-checked the roofline.py conflict resolution against #1970's recorder API, the Python 3.10/3.11 imports and the gate totals (397 -> 331 function-args rows) at head. Checked: the rebase delta against the clean r2 review (the +/- lines of the PR diff differ only in the
LGTM |
|
/review |
|
Automated review started for head 3e4596d. Progress: https://github.com/AMD-AGI/Hyperloom/actions/runs/39193259430 |
PR #1972 — refactor(orchestrator): pass actions, state and trace-analysis inputs as typed objects instead of parallel keywordsWhat it does: 95 functions under Blocking issues: none Checked:
Ran: LGTM |
xiaofei-zheng
left a comment
xiaofei-zheng
left a comment
There was a problem hiding this comment.
No blocking issues. Review details in the comment above.
|
Automated review: no verdict applied -- the PR is MERGED. Comment PR #1972 -- refactor(orchestrator): pass actions, state and trace-analysis inputs as typed objects instead of parallel keywordsWhat it does: Functions in Blocking issues: none Step 7 independent reader: a second reader re-ran the whole adversarial pass against card.md, diff.txt and base.txt only -- tree-wide greps for surviving old-shape callers of every reshaped or removed symbol, the roofline merge into #1970's ProfileRunOutcome/AnalysisRunOutcome, the dataclass defaults, the Ray-boundary pickling and the baseline count (397 -> 331), plus head test runs (round_store, bypass backend/reader/report, selfcert, reconcile, setup replay, apply feedback, ray backend unit, enablement keep records, integrate-patch coverage unit -- all green); no blocking issue found, and the one candidate it raised (test_trace_shape_manifest collection error) reproduces identically on the base tree (missing TraceLens in the sandbox, not a regression). SKIPPED: V4 -- "# PARTIAL: #1032 changes 1075 files, only its first 100 were compared", overlap there unchecked Checked: all 42 changed production files, including the Tier 1 |
Rebased onto main after #1970 and #1971; the one conflict (
actions/executors/roofline.py) keeps both refactors:_end_profile_run/_end_analysis_runtake this PR's_RunEndand build #1970'sProfileRunOutcome/AnalysisRunOutcome. #1973 shares 4 call-site files with this PR and rebases after it.Description: what and why
Brings
orchestrator/actions,orchestrator/stateandorchestrator/trace_analysisunder the parameter-count limit (function-args> 5). Many of these functions were helpers extracted earlier from large executors and took long lists of the caller's locals, or took the fields of one row, request or command as separate keywords. Each now takes the object those values belong to:AttemptRow(one functionrecord_attempt_rowwrites both config and patch attempt rows, replacing_record/record_config_attempt/record_patch_attempt),RoundRequest,TaskSpec,KbInjection;record_tick_exceptiontakes the exception; the roofline snapshot history reads thelast_trace_analyzeentry written just before it.TraceLensSkillRequest,SdkEntryPoints,RunStatus(update_statusbecomesRunStatus.write),TraceInputs,AuditSubject,OrchestratorOutcome,TraceSplitJob,AnalysisReport,BypassReportContext,KernelAggregate,TraceAnalysisOptions,GraphVariant,CaptureShard,ManifestOrigin,WorkloadTotals,TraceHealth,CoverageMeasures.ServerSpec/ClientRun/EvalRun/HealthPoll/LifecycleRecord,ReportSubject/RunOutcome,ScriptableBenchmark; session-killSessionKillRound,_WatchedRound,ActorLaunch; integrate-patch_PatchBench,_PatchMeasurement,_KbLesson; roofline_RunEnd,_AnalysisAttempt;_TraceProbe,_AnalysisRun,_GeakPointReport,_SiteTiming,GapEvidence,_SourcePatch,_PatchMode,SetupLedger.populate_gpu_arch_jsonbetween two contracts is split intointernal_extension_arch_json.No
**kwargs, untyped dict, compatibility shim or new fallback branch is added; every call site is updated.Scope: 95 functions are over the limit in those three directories. 13 of them sit in four files that other open PRs are restructuring and that this PR does not touch (
_grid_runner.py4,_workload_envs.py2,baseline.py6,explore.py1). Of the other 82, 66 are done, so 29 remain over the limit in these directories after this PR. The 16 in touched or callable files that keep their signatures:resolve_graded_comparison,SharedState.record_policy_denial,SharedState.record_action_attempt,TaskRegistry.create_or_return_existingand its twinTaskRegistry.create,sweep_via_geak,GridVariant.__init__,apply_compatibility_filter,effective_fingerprint,build_launch_evidence,run_with_session_kill,restart_server_for_round,ServingLease.run_session_kill); none of those files is touched here.KvMetricsRecorder.__init__(its module is already past the 1200-line ceiling),validate_session_shape(the type would have to be imported from a private module across packages, rule A1),GpuSpecialistLease.start_async(its only caller is exactly at its length and cognitive-complexity baseline; the layer below,ServingActor.start, now takesActorLaunch).Gate count of functions over the parameter limit (
function-argsentries inscripts/code_metrics_baseline.txt): 397 on main -> 331 (-66); the cyclomatic-complexity entries and the broad-exceptcount are unchanged.Linked issue(s): none.
Tests: added/updated? commands run?
test_tracelens_csv.pya 9-field request block repeated 15 times is built by one helper.iter_details_from_name, the bypass server / client / eval argv, the report JSON and the pid / meta lifecycle files.PYTHONPATH=src(never the full suite): all 44 changed test files and every other test file that calls or references a changed signature (118 files after the rebase onto main, 5505 passed). Every changed production module imports underpython:3.10-slimandpython:3.11-slim. TraceLens-dependent tests ran against TraceLens at the CI-pinned commit647cbf27, unpacked outside the tree.ruff check/ruff format --checkon the changed files;python scripts/code_metrics.py --base-ref origin/mainPASSED; baseline lowered with--update-baseline(1348 -> 1277 entries).Size/complexity triggers crossed: split, or why the shape is right:
Diff budget: 3540 production Python lines changed (+1748 / -1792 in 42 production
.pyfiles). One concern (parameter count of these three orchestrator directories and their call sites). Each commit touches at most 5 production files and is grouped by module. The code-metrics gate passes without a waiver.If this simplifies or refactors: which mechanism was removed, which contract was preserved, and the tests that show it:
Removed: the parallel keyword lists and the three attempt-row writers (one remains). Preserved:
state.jsonand every persisted row and file, executor argv / env / paths, name-resolved entry points (Coordinator collaborators,ACTION_CATALOGUE,KERNEL_REQUEST_HANDLERS), shown by the tests and byte comparisons above.Deleted parameters (checked across src, scripts, .github, docs, examples and tests):
build_shape_manifest(tracelens_revision=)(never passed) andanalysis_route=(its only caller passes the default),populate_gpu_arch_json(device=),build_eval_command(batch_size=),build_apply_feedback(rejected_hunks=)(never passed),build_client_command(result_filename=)(every caller passes"inferencex_result"and the reader readsinferencex_result.json),_run_scriptable_benchmark(bench_envs=)(passed but never read),_build_analysis_meta(tool=)(derived one-to-one fromroute, as the handler already did).Observable effect: what an operator will now see, and what the old behaviour cost them (if none, why the change is unobservable):
None: no output file,
state.jsonfield, CLI flag, command line or log line changes.Breaking changes: yes, for Python callers only: the refactored functions take their typed objects. Every in-tree caller is updated.
PR addresses single concern: yes (parameter count of
orchestrator/actions,stateandtrace_analysis).Root cause is upstream (Magpie/TraceLens/GEAK/IntelliKit/AgentKernelArena), ticket filed: n/a.