Skip to content

Update dsr1-fp8-b200-sglang SGLang image to v0.5.11-cu130#1322

Open
Klaud-Cold wants to merge 1 commit into
mainfrom
claude/issue-1154-dsr1-fp8-b200-sglang
Open

Update dsr1-fp8-b200-sglang SGLang image to v0.5.11-cu130#1322
Klaud-Cold wants to merge 1 commit into
mainfrom
claude/issue-1154-dsr1-fp8-b200-sglang

Conversation

@Klaud-Cold
Copy link
Copy Markdown
Collaborator

Summary

  • Update dsr1-fp8-b200-sglang image from lmsysorg/sglang:v0.5.9-cu130 to lmsysorg/sglang:v0.5.11-cu130

Ref #1154

Generated with Claude Code

…\n\nCo-authored-by: Klaud Cold <Klaud-Cold@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor

Thanks for the contribution! For vLLM & SGLang, please ensure that your recipes is similar to the official vLLM recipes and/or the SGLang cookbook

If it is not, please create a PR first before we can merge your single node PR into the master branch. Let's ensure that the documentation is first class such that the entire ML community can benefit from your hard work! Thank you

PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. If re-running failed jobs is attempted, PR authors are responsible for ensuring it passes. See GitHub's docs on re-running failed jobs: https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs#re-running-failed-jobs-in-a-workflow

As a rule of thumb, generally, PR authors should request a review & get a PR approval from the respective companies' CODEOWNERS before requesting a review from core maintainers.

If additional help is needed, PR authors can reach out to core maintainers over Slack.

1 similar comment
@github-actions
Copy link
Copy Markdown
Contributor

Thanks for the contribution! For vLLM & SGLang, please ensure that your recipes is similar to the official vLLM recipes and/or the SGLang cookbook

If it is not, please create a PR first before we can merge your single node PR into the master branch. Let's ensure that the documentation is first class such that the entire ML community can benefit from your hard work! Thank you

PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. If re-running failed jobs is attempted, PR authors are responsible for ensuring it passes. See GitHub's docs on re-running failed jobs: https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs#re-running-failed-jobs-in-a-workflow

As a rule of thumb, generally, PR authors should request a review & get a PR approval from the respective companies' CODEOWNERS before requesting a review from core maintainers.

If additional help is needed, PR authors can reach out to core maintainers over Slack.

Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — simple SGLang image version bump with a corresponding changelog entry; the only finding is the placeholder PR link in the changelog noted inline.

Extended reasoning...

Overview

This PR is a two-line change: it bumps the dsr1-fp8-b200-sglang config's image from lmsysorg/sglang:v0.5.9-cu130 to lmsysorg/sglang:v0.5.11-cu130 in .github/configs/nvidia-master.yaml, and appends a matching entry to perf-changelog.yaml describing the bump.

Security risks

None. This is a benchmark/perf config file referencing a public upstream image tag in the same registry/repo as the prior pin. No auth, crypto, permissions, or runtime code paths are touched.

Level of scrutiny

Low. This is a mechanical config tweak following an established pattern (many prior PRs in the changelog do exactly this), with no logic changes. The only finding is a documentation nit (a placeholder XXX in the changelog pr-link) which is already surfaced as an inline comment and is trivially fixable post-merge if needed.

Other factors

The full-sweep-enabled label indicates CI will run the full benchmark sweep to validate the new image works against the existing recipe — the PR author is responsible for making sure that sweep passes per the recipe-reminder bot. The placeholder PR link is a minor doc issue, not a blocker for the underlying config change.

Comment thread perf-changelog.yaml
- dsr1-fp8-b200-sglang
description:
- "Update SGLang image from v0.5.9-cu130 to v0.5.11-cu130"
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The new perf-changelog entry has pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX where XXX is an unfilled placeholder rather than this PR's number. Since this is PR #1322, the link should be pull/1322 — as written, it resolves to a 404 and breaks the changelog cross-reference. Trivially fixable by replacing XXX with 1322.

Extended reasoning...

The bug

The new entry appended to perf-changelog.yaml (line 2351) sets:

pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX

The XXX is clearly a template placeholder that was never substituted with this PR's real number. Every other entry in the file uses a real numeric PR id, for example the immediately preceding entries:

  • pull/1304
  • pull/1305
  • pull/1308
  • pull/1310

Why it matters

The changelog is a discoverability/audit aid — given a config-keys row, a reader follows pr-link to see why the configuration changed (here: the SGLang image bump from v0.5.9-cu130 to v0.5.11-cu130). With XXX left in place, that cross-reference is permanently broken: GitHub will return a 404 for /pull/XXX, so the rationale becomes unreachable from the changelog.

Why existing code doesn't prevent it

perf-changelog.yaml is free-form YAML. There is no schema check or lint that validates pr-link points at an existing PR (or even that the path component is numeric), so a placeholder slips through both human review and CI.

Step-by-step proof

  1. Open perf-changelog.yaml at the bottom of the file (the diff hunk in this PR).
  2. The new entry, lines 2347–2351, reads:
    - config-keys:
        - dsr1-fp8-b200-sglang
      description:
        - "Update SGLang image from v0.5.9-cu130 to v0.5.11-cu130"
      pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/XXX
  3. The PR metadata for this change is #1322 (title: "Update dsr1-fp8-b200-sglang SGLang image to v0.5.11-cu130").
  4. Compare with the entry immediately above (lines 2340–2345):
    pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1310
    — numeric, resolves to a real PR.
  5. Visiting https://github.com/SemiAnalysisAI/InferenceX/pull/XXX returns a 404 (no such PR id), confirming the link is dead.

Fix

Replace XXX with 1322:

pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1322

Severity is nit: this is a broken doc link, not a runtime/functional issue, but it should be fixed before merge so the changelog stays internally consistent.

@github-actions
Copy link
Copy Markdown
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant