Repository navigation
chore: remove Kubernetes deployment path (Helm chart, trainer metrics server) - #3767
Merged
Merged
Conversation
faresobeid
marked this pull request as ready for review
October 3, 2026 01:18
faresobeid
force-pushed
the
chore/remove-k8s
branch
from
October 3, 2026 22:59
a1ef815 to
42e89b4
Compare
This was referenced Oct 4, 2026
JannikSt
approved these changes
Oct 4, 2026
JannikSt
approved these changes
Oct 4, 2026
samsja
pushed a commit
that referenced
this pull request
Oct 4, 2026
Removes duplicate and unused entries from `pyproject.toml` (rev-infra P9). The set of installed packages does not change for any extra combination (checked below). | Removed | Reason | |---|---| | second `orjson>=3.10` in `dependencies` | Duplicate of `orjson>=3.11.0` a few lines above. | | `pandas>=2.0` | Nothing imports it: no `import pandas` / `pd.` in `src/`, `tools/`, `scripts/`, `tests/`, `examples/`, `skills/`, `packages/`. It was added for the nano bridge (7089ba1). It stays installed because `datasets` depends on it. | | `pyarrow>=21.0.0` | No direct use anywhere in those directories. The one parquet writer, `tools/convert_traces_to_hf_dataset.py`, calls `datasets.Dataset.to_parquet`, and `datasets` depends on `pyarrow` itself. It stays installed. | | `nixl-cu13==0.10.1` in the `disagg` extra | `disagg` already lists `nixl`, and the `[[tool.uv.dependency-metadata]]` override for `nixl` 0.10.1 requires exactly `nixl-cu13==0.10.1`. The `gpu` extra keeps its explicit pin, because `transports/weights/nixl/agent.py` imports `nixl_cu13` directly. | | `mamba-ssm` dependency group | Duplicate of the `gpu` extra entry, which was added with the native Nemotron-H model (#3426). Every user of the group also syncs `--extra gpu` or `--all-extras`, so I dropped `--group mamba-ssm` from `Dockerfile.cuda` (2 places), `scripts/docker-entrypoint.sh` and the `pre_run_command` in `configs/advanced/nemotron-3-super/swe.toml`. The `[tool.uv.sources]` git pin and the build dependencies for mamba-ssm stay. | Behavior change: - None for installs: same locked packages and versions. - `uv sync --group mamba-ssm` no longer works; use `--extra gpu`, which every repo caller already passes. - `pandas` and `pyarrow` lose their direct lower bounds. They are still pulled in by `datasets`. ## Checks - `uv lock`: resolves 564 packages, the same set as before (568 `[[package]]` entries before and after). `uv lock --check` passes. The `uv.lock` diff only touches prime-rl's own metadata. I reverted the reordering of `[options.exclude-newer-package]` that my uv version (0.12.10) introduced. - I ran `uv export --frozen --all-packages` and compared the package lists before and after, ignoring the `# via` comment lines. They are identical for: no extras, `--extra disagg`, `--extra gpu`, `--extra dashboard`, `--all-extras`, and `--all-extras --all-groups`. `--extra disagg` still yields `nixl==0.10.1` and `nixl-cu13==0.10.1`, and `--extra gpu` still yields `mamba-ssm` at the pinned rev. - No Python changes, so no unit tests are affected. GPU: not needed. The Docker image build in CI covers the Dockerfile and entrypoint edits. ## Conflicts Text conflicts only in `pyproject.toml` / `uv.lock`, as with any dependency PR. None of the open PRs that touch these files (e.g. #3829, #3803, #3767, #3660) edit the lines removed here. A re-lock after rebase is enough. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Metadata and sync-flag cleanup only; lockfile exports are unchanged and runtime installs should match prior behavior. > > **Overview** > Cleans up **duplicate and redundant dependency declarations** in `pyproject.toml` and aligns `uv.lock`, Docker, and Slurm sync commands with the slimmer manifest. **No intended change to installed packages** for existing extra combinations (`gpu`, `disagg`, `--all-extras`, etc.). > > **Removed from core `dependencies`:** duplicate `orjson>=3.10`, unused direct pins on `pandas` and `pyarrow` (they still come in via `datasets`). **Removed from `disagg` extra:** explicit `nixl-cu13==0.10.1` because `nixl` metadata already pulls that version; the **`gpu` extra keeps** its explicit `nixl-cu13` pin for direct imports. **Dropped the `mamba-ssm` dependency group** as a duplicate of the `gpu` extra entry—`Dockerfile.cuda`, `scripts/docker-entrypoint.sh`, and the Nemotron SWE `pre_run_command` no longer pass `--group mamba-ssm`. > > **Workflow note:** `uv sync --group mamba-ssm` alone is no longer supported; use `--extra gpu` (or `--all-extras`), which callers already use. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit f0f2ff6. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
faresobeid
added a commit
that referenced
this pull request
Oct 6, 2026
The dashboard smoke check at the end of the `alphabet_sort` integration test flakes at `tests/integration/dashboard_smoke.py:164`. It asserts that `#replay-top` is active after Home. It failed on #3767 and #3774, which don't touch the dashboard. **Root cause.** The smoke test presses `t` and then Home. When thinking is hidden, a trace that overflowed the replay pane gets shorter and fits. The browser then clamps `scrollTop` and queues a scroll event. That event fires after Home. At that point the pane fits, so the scroll handler sees the view as "at bottom", switches back to follow mode, and clears `#replay-top`. The same thing happens when the pane overflows by less than the 24px threshold, because top and bottom then count as the same position. **Fix.** When the replay output fits (overflow < 24px), the scroll handler leaves the follow mode alone. Home and End decide it, and that choice still applies if the output grows later. Scrolling a long trace by hand still switches between the modes as before. **Check.** I ran headless Chromium against a dashboard serving synthetic runs, with the same steps as the smoke test (skip inference, `t`, Home, End): - trace that fits after hiding thinking: 4/4 failed before the fix, 4/4 passed after - 14px overflow: 4/4 failed before the fix, 4/4 passed after - short trace that always fits: passed before and after - long scrollable trace: passed before and after - long trace, scrolling up and back down by hand: still toggles follow mode and the top/live buttons <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Small dashboard UI scroll-handler guard with no auth, data, or API impact. > > **Overview** > Fixes flaky replay **Home/End** behavior when the trace output **fits** (or barely overflows) in the replay pane. > > The `#replay-output` scroll listener now **bails out** when `scrollHeight - clientHeight < 24`, so clamped or near-no-op scroll events (e.g. after hiding thinking shrinks the content) no longer force **follow/live** mode and clear **`#replay-top`**. **Home/End** and manual scrolling on long traces are unchanged. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 83d2f3f. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
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.
Removes the Kubernetes deployment path. The Helm chart has had no functional change since its initial additions (last own change 2026-08-12 was a docs fix), and supported multi-node deployment is SLURM (see docs/scaling.md). The trainer
metrics_server(Prometheus/metrics+/health) only existed to serve the chart's liveness/readiness probes.Removed
k8s/Helm chart (andCOPY k8sfromDockerfile.cuda)src/prime_rl/utils/metrics_server.py(MetricsServer,HealthServer) and its unit testMetricsServerConfig(configs/shared.py), trainermetrics_serverfield, and its setup/update/teardown intrainer/rl/train.pyexamples/basic/reverse-text/README.md,docs/training.md;k8s/TOMLs dropped fromtest_configs.pyliveness_timeout_secondsdocstring reworded neutrally; the/livenessendpoint stays (used by SLURM/router health checks)prometheus-clientstays inpyproject.toml:orchestrator/inference_metrics.pystill uses its parser.Breaking changes
metrics_servernow fail validation./app/k8sin the image are gone.Validation
pytest tests/unit/test_configs.py -m "not gpu"(205 passed)ruff check .andruff format --check .Note
Medium Risk
Breaking for anyone using the Helm chart or trainer
metrics_serverconfig; training/runtime core paths are unchanged but deployment and observability options shrink.Overview
Removes the Kubernetes/Helm deployment path and related trainer observability that existed mainly for chart probes.
The entire
k8s/Helm chart (StatefulSets, services, PVC, reverse-text example values/TOMLs, andk8s/README.md) is deleted.Dockerfile.cudano longer copiesk8s/into the image, and docs/README/example copy now describe Slurm-only multi-node deployment (K8s sections dropped fromreverse-textREADME).Trainer Prometheus/health HTTP server is removed:
MetricsServerConfig, themetrics_servertrainer config field,src/prime_rl/utils/metrics_server.py, RL trainer startup/update/shutdown hooks, and unit tests. Configs that still setmetrics_serverwill fail validation.Minor doc tweaks: inference
liveness_timeout_secondshelp text is generic (not K8s-specific);env-serverdocs refer to externally managed hosts instead of pods. Config validation tests only scanconfigs/andexamples/TOMLs, notk8s/.Inference
/livenessandprometheus-clientelsewhere (e.g. orchestrator) are unchanged.Reviewed by Cursor Bugbot for commit 42e89b4. Bugbot is set up for automated code reviews on this repo. Configure here.