Skip to content

ci: update container registry location - #3

Merged
saturley-hall merged 16 commits into
mainfrom
harrison/ci-test
Mar 4, 2025
Merged

ci: update container registry location#3
saturley-hall merged 16 commits into
mainfrom
harrison/ci-test

Conversation

@saturley-hall

@saturley-hall saturley-hall commented Mar 4, 2025

Copy link
Copy Markdown
Member

What does the PR do?

Updates the container registry used for caching

Splits CodeQL and Copyright-Checks into separate workflows.

Update image that Copyright checks uses to a new location in GHCR.io

@github-actions

github-actions Bot commented Mar 4, 2025

Copy link
Copy Markdown
Contributor

Test Results

 2 files   2 suites   25s ⏱️
71 tests 71 ✅ 0 💤 0 ❌
89 runs  88 ✅ 1 💤 0 ❌

Results for commit c7118b9.

♻️ This comment has been updated with latest results.

furionw added a commit that referenced this pull request May 12, 2026
…later

Reader-first ordering. Old order buried Quick start at position 8 of 9;
new order surfaces the runnable commands above all the reference
sections (FSx vs EBS, label conventions, aiperf SHA rationale).

New section order:
  1. Title + 3-config table
  2. Pre-requisites           (was #3)
  3. Quick start              (was #8 — promoted)
  4. Directory layout         (was #7 — now serves as map for the rest)
  5. Hardware targets         (was #2 — now pure reference; invocation
                                       examples moved into Quick start)
  6. Storage                  (was #5)
  7. aiperf install           (was #6)
  8. Naming & ownership       (was #4)
  9. Notes                    (unchanged)

Also drop the stray "We use ebs by default" sentence — it contradicted
both the Storage section and the actual yaml (where the PVC block is
fully commented out, no default storage class is set).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tanmayv25 added a commit that referenced this pull request May 16, 2026
…cycle

Address senior-architect review of the metrics infrastructure:

#1 (two parallel Prometheus paths): merge `register_prometheus` +
`setup_component_metrics` into one `setup_metrics(ctx) -> MetricsBindings`
method on the Rust `LLMEngine` trait. The trait surface shrinks from
7 → 6 methods; both expfmt bridging and the structured component
publisher go through one place. The Python ABC keeps its two methods —
the PyO3 bridge adapts (engine-author API unchanged for Python).

#3 + #8 (framework lifecycle gauges gated on engine opt-in):
- `cleanup_time_seconds` and `drain_time_seconds` move to Rust-side
  `LifecycleGauges` (prometheus crate gauges registered via the
  runtime's `MetricsRegistry`). Worker constructs them after
  `engine.start()` succeeds; observes during cleanup/drain. Emits
  regardless of engine opt-in.
- Drop `set_cleanup_time` / `set_drain_time` from the
  `ComponentMetricsPublisher` trait — engine no longer responsible.
- Drop the corresponding methods from `PyComponentMetricsPublisher`.
- `model_load_time_seconds` STAYS on Python `LLMBackendMetrics` for
  parity with the legacy entry points (both legacy `main.py` and the
  unified bridge populate it). The unified bridge now constructs
  `LLMBackendMetrics` UNCONDITIONALLY so the gauge emits even when
  the engine returns no per-rank sources.

#5 (kv_cache_hit_rate Optional semantics): clearer docstring on both
Rust `ComponentSnapshot` and the Python ABC — tri-state explicitly
documented (`None` = no data, `0.0` = legitimate zero-hit).

Deferred with rationale (explained in the post-review architect's note):
- #2 (HashMap<MetricKey, f64> ComponentSnapshot): real future-proofing
  but no current bug; 5 fields for 5 signals isn't worth the refactor
  cost yet. Revisit at ~10+ signals.
- #4 (per-source cadence): explicit defer in original review.
- #7 (conformance in CI): workflow change, separate from trait surface.

Conformance kit collapses `check_register_prometheus` +
`check_component_metrics` into `check_setup_metrics`.

All 68 backend-common unit tests pass. Mocker example updated.
PyO3 bridge wheel rebuilt successfully.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kaim-eng added a commit that referenced this pull request May 22, 2026
When rbac.namespaceRestricted=true the chart renders a namespace-scoped
Role + RoleBinding (templates/role.yaml + rolebinding.yaml) instead of
the cluster-scoped pair, but the DaemonSet's container command never
passes --namespace to the agent. power_agent.py:438-446 therefore takes
its else branch — list_pod_for_all_namespaces — which the namespace-only
token rejects with 403 Forbidden on every reconcile. Reconciles never
advance and no caps are ever applied (silent failure: the agent doesn't
exit; pods are just never enumerated).

The --namespace CLI flag itself already exists (power_agent.py:542) and
the list_namespaced_pod branch (:439-442) works correctly — the dev-pod
template (templates/dev-pod.yaml:57) has been wiring this all along via
--namespace=$POD_NAMESPACE. This commit extends the same downward-API
POD_NAMESPACE env-var pattern to the production DaemonSet, gated on the
existing power-agent.effectiveNamespaceRestricted helper so both:

  - production mode with rbac.namespaceRestricted=true
  - dev mode (forces effective=true unless overridden)

produce a DaemonSet whose argv matches the RBAC scope its token holds.

Verified by helm template against both modes:
  - default (false) → no --namespace, no POD_NAMESPACE, ClusterRole
  - true            → --namespace=$(POD_NAMESPACE), POD_NAMESPACE env
                      via downward API, Role + RoleBinding
  - helm lint passes in both modes

Refs: PR #9682 review, Power Agent live-test finding #3.
Signed-off-by: Kai Ma <kaim@nvidia.com>
kaim-eng added a commit that referenced this pull request May 25, 2026
When rbac.namespaceRestricted=true the chart renders a namespace-scoped
Role + RoleBinding (templates/role.yaml + rolebinding.yaml) instead of
the cluster-scoped pair, but the DaemonSet's container command never
passes --namespace to the agent. power_agent.py:438-446 therefore takes
its else branch — list_pod_for_all_namespaces — which the namespace-only
token rejects with 403 Forbidden on every reconcile. Reconciles never
advance and no caps are ever applied (silent failure: the agent doesn't
exit; pods are just never enumerated).

The --namespace CLI flag itself already exists (power_agent.py:542) and
the list_namespaced_pod branch (:439-442) works correctly — the dev-pod
template (templates/dev-pod.yaml:57) has been wiring this all along via
--namespace=$POD_NAMESPACE. This commit extends the same downward-API
POD_NAMESPACE env-var pattern to the production DaemonSet, gated on the
existing power-agent.effectiveNamespaceRestricted helper so both:

  - production mode with rbac.namespaceRestricted=true
  - dev mode (forces effective=true unless overridden)

produce a DaemonSet whose argv matches the RBAC scope its token holds.

Verified by helm template against both modes:
  - default (false) → no --namespace, no POD_NAMESPACE, ClusterRole
  - true            → --namespace=$(POD_NAMESPACE), POD_NAMESPACE env
                      via downward API, Role + RoleBinding
  - helm lint passes in both modes

Refs: PR #9682 review, Power Agent live-test finding #3.
Signed-off-by: Kai Ma <kaim@nvidia.com>
krishung5 added a commit that referenced this pull request Jun 2, 2026
- decode_handler._extract_mm_hashes docstring now states 16-char hex
  for the SGLang path (was incorrectly claiming the vLLM 64-char shape;
  Devin #5).
- sglang launch banner drops the "Lightseek" prefix; uses "MM Exact
  Routing (SGLang)" to match the public name (Ryan suggestion #1).
- ModelRuntimeConfig.backend_framework field gains a doc-block on its
  motivation — frontend uses it for backend-specific routing hints
  (Ryan #3).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kaim-eng added a commit that referenced this pull request Jun 3, 2026
When rbac.namespaceRestricted=true the chart renders a namespace-scoped
Role + RoleBinding (templates/role.yaml + rolebinding.yaml) instead of
the cluster-scoped pair, but the DaemonSet's container command never
passes --namespace to the agent. power_agent.py:438-446 therefore takes
its else branch — list_pod_for_all_namespaces — which the namespace-only
token rejects with 403 Forbidden on every reconcile. Reconciles never
advance and no caps are ever applied (silent failure: the agent doesn't
exit; pods are just never enumerated).

The --namespace CLI flag itself already exists (power_agent.py:542) and
the list_namespaced_pod branch (:439-442) works correctly — the dev-pod
template (templates/dev-pod.yaml:57) has been wiring this all along via
--namespace=$POD_NAMESPACE. This commit extends the same downward-API
POD_NAMESPACE env-var pattern to the production DaemonSet, gated on the
existing power-agent.effectiveNamespaceRestricted helper so both:

  - production mode with rbac.namespaceRestricted=true
  - dev mode (forces effective=true unless overridden)

produce a DaemonSet whose argv matches the RBAC scope its token holds.

Verified by helm template against both modes:
  - default (false) → no --namespace, no POD_NAMESPACE, ClusterRole
  - true            → --namespace=$(POD_NAMESPACE), POD_NAMESPACE env
                      via downward API, Role + RoleBinding
  - helm lint passes in both modes

Refs: PR #9682 review, Power Agent live-test finding #3.
Signed-off-by: Kai Ma <kaim@nvidia.com>
jthomson04 added a commit that referenced this pull request Jun 4, 2026
The cache-realloc (#1) and metric-handle (#2/#3) commits left a few lines
unformatted — `longest_prefix_match`'s collapsed signature and the `merged`
binding in l1.rs, and the per-worker gauge assignment + a test assert in
metrics.rs. `cargo fmt --all --check` (run by the rust-tests CI job) flagged
them, failing the job. No behavior change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jthomson04 added a commit that referenced this pull request Jun 4, 2026
The cache-realloc (#1) and metric-handle (#2/#3) commits left a few lines
unformatted — `longest_prefix_match`'s collapsed signature and the `merged`
binding in l1.rs, and the per-worker gauge assignment + a test assert in
metrics.rs. `cargo fmt --all --check` (run by the rust-tests CI job) flagged
them, failing the job. No behavior change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kangclzjc referenced this pull request in kangclzjc/dynamo Jun 4, 2026
… path (review #3)

On the orchestrator replay path (use_orchestrator=True) the regression
models were never installed: replay/main.py guarded the AIC benchmark-FPM
bootstrap behind ``adapter._sm is not None and not adapter._sm._is_easy``,
and ``_sm`` is None under use_orchestrator — so the whole block (incl. the
``load_benchmark_fpms`` calls) was skipped. ``get_regression`` then returned
None for the whole replay, the throughput regression stayed empty, and
orchestrator-replay scaling decisions diverged from PSM — contradicting the
adapter docstrings that claimed bootstrap_from_fpms→install_regressions was
wired.

Fix:
- engine_adapter: extract ``install_regressions_from_fpms`` (synchronous,
  builds the regressions from benchmark FPMs via the throwaway-PSM factory
  and installs them on the shared store — no plugin bootstrap).
  ``bootstrap_from_fpms`` now = install_regressions_from_fpms +
  bootstrap_plugins.
- replay_adapter: add path-agnostic ``install_benchmark_fpms`` — PSM path →
  ``PlannerStateMachine.load_benchmark_fpms``; orchestrator path →
  ``install_regressions_from_fpms`` (plugins were already bootstrapped at
  adapter construction, so this does NOT double-bootstrap). Corrected the
  ``_get_regression`` docstring to the real wiring.
- replay/main.py: guard is now ``not adapter._is_easy_mode()`` (path-
  agnostic) and the two ``adapter._sm.load_benchmark_fpms(...)`` calls become
  ``adapter.install_benchmark_fpms(...)``.

Test: install_benchmark_fpms on the orchestrator path makes
get_regression("agg") non-None (was None pre-fix).

840 planner tests pass (+1). NOTE: replay/main.py's end-to-end path can't be
exercised in this env (its Rust _core symbol predates the installed
extension); the edit is py_compile-validated and the adapter method it calls
is unit-tested.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jthomson04 added a commit that referenced this pull request Jun 4, 2026
The cache-realloc (#1) and metric-handle (#2/#3) commits left a few lines
unformatted — `longest_prefix_match`'s collapsed signature and the `merged`
binding in l1.rs, and the per-worker gauge assignment + a test assert in
metrics.rs. `cargo fmt --all --check` (run by the rust-tests CI job) flagged
them, failing the job. No behavior change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
zhongdaor-nv added a commit that referenced this pull request Jun 8, 2026
During each cell's measurement window, take a few lightweight per-node
snapshots (background thread, foreground sub.wait() unperturbed) capturing the
event-plane's resource footprint so we can quantify the brokerless-ZMQ vs NATS
cost trade-off:

  - established TCP count via `ss -tn state established` (ZMQ direct mesh is
    O(p*s); NATS is O(p+s) to the broker) -- the headline connection-cost diff.
  - per-process CPU% / RSS / open-fd / nproc for the key classes
    (mocker, event_plane_bench_sub, nats-server, frontend, zmq_broker, loadgen)
    via /proc/<pid>/{stat,status} + ls /proc/<pid>/fd.
  - nats-server broker cost: active during NATS cells vs ~idle during ZMQ
    cells (ZMQ has no broker -- the saving to quantify).

New ressample.py is a stdlib-only host-level sampler (runs on the node HOST,
not inside pyxis, so its ss/`/proc` scan sees every enroot process + all node
TCP). bench.py fires it on every node via a new launcher.popen_host() (srun
with no --container-* flags), collects the echoed JSON snapshots, folds them
into compact summary stats, writes a per-cell *_res.json, and attaches the
summary to every summary.json row. Gated behind DYN_BENCH_RESSAMPLE (default
on); snapshot count via DYN_BENCH_RESSAMPLE_N (default 2).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
zhongdaor-nv added a commit that referenced this pull request Jun 8, 2026
The sampler snapshots are read back from the srun-streamed stdout logs, not a
follow-up `cat` srun; correct the docstring to match the implementation.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
zhongdaor-nv added a commit that referenced this pull request Jun 8, 2026
#3)

Three fixes found in the lego-c2 sanity run:

1. RSS read 0 for in-container processes. Viewed from the node host, enroot/
   pyxis processes report VmRSS=0 in /proc/<pid>/status (and 0 resident in
   statm); only /proc/<pid>/smaps_rollup `Rss:` reports the true resident set.
   Read smaps_rollup first, fall back to VmRSS for host procs.

2. nproc inflation. The `srun` client + slurmstepd that launch nats-server /
   event_plane_bench_sub carry the full `-- nats-server ...` command in their
   argv, so the cmdline-substring match counted them too (nats nproc=3 instead
   of 1, with 0 RSS dragging the sum). Skip launcher/wrapper cmdlines
   (srun/slurmstepd/bash -c/` -- `/the sampler itself) so each class counts
   only the real target process.

3. Worker-node samplers crashed with FileNotFoundError: the node0-created
   LOGDIR doesn't exist on worker nodes, so `open(--out)` died before the
   snapshot was emitted -> only node0 reported, losing the cross-node
   connection counts. Now echo the JSON to stdout FIRST (the orchestrator
   reads snapshots from the srun-streamed stdout, not the --out file), then
   makedirs + best-effort write the node-local --out copy.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BenHamm added a commit that referenced this pull request Aug 20, 2026
- SPDX headers on synthesize-user-workload openai.yaml and all .codex files (#1)
- CODEOWNERS entries for .codex/, agents/, agent-docs/ (#2)
- lycheeignore the two agent-docs links that 404 until this PR merges (#3)
- .gitignore: remove local-dev leftovers that ignored two tracked skill
  dirs; keep an anchored /runs/ for EXP_ROOT artifacts (#4-7)
- vllm.md: scope the parallelism constraint to gpus_per_replica (#10)
- tensorrt-llm.md: drop engine-era/C++-executor KV fields and use_cuda_graph
  in favor of the surfaces real configs use (#11)
- vllm.md: normalize flag spellings to hyphenated form; hedge the
  unverifiable v0.18 scheduler-default claim (#12)
- kubernetes-recipe-workflow.md: define DEPLOY_ROOT before use (#14)

agent-reported label created in the repo (#13). Findings 8-9 (stop-request
record location) tracked separately pending a design decision.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants