fix(cluster): strip the accelerator_type prefix, not its characters - #173
Draft
nilsmechtel wants to merge 1 commit into
Draft
fix(cluster): strip the accelerator_type prefix, not its characters#173nilsmechtel wants to merge 1 commit into
nilsmechtel wants to merge 1 commit into
Conversation
_get_accelerator_type used str.lstrip("accelerator_type:"), which strips
any leading character in that set rather than the literal prefix. It is
correct for A40, T4 and G only because each starts with an out-of-set
character; "accelerator_type:tesla" yields "sla".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
_get_accelerator_typeusedstr.lstrip("accelerator_type:")to remove a prefix.lstriptakes a character set, not a prefix, so it strips every leading character that appears anywhere in{a, c, e, l, r, t, o, _, y, p, :}. The line is correct today only by luck —A40,T4,GandGeForce-RTX-3090each begin with a character outside that set. Any accelerator type starting with one of them is silently mangled:removeprefixis the operation that was meant. Python ≥3.11 is already required (pyproject.toml:9), so it is available unconditionally.tests/test_accelerator_type.pypins it: an ordinary type is read back unchanged, a type whose leading characters all appear in the prefix survives intact, and a node with no accelerator resource still yieldsNone. A positive control was run — restoringlstripfails exactly the middle test and leaves the other two passing, so the new test discriminates rather than passing for free.I checked whether this mistake appears elsewhere before fixing the one line: every other
lstrip/rstripinbioengine/(apps/builder.py:224,datasets/http_zarr_store.py:119,120,149,294,datasets/datasets.py:153,312,453,worker/worker.py:240) strips"/", a genuine single-character set, which is what the method is for. This was the only misuse.The
accelerator_type: "G"display is upstream, and the obvious workaround does not workThis PR does not change what the single-machine Europa worker reports for a GeForce RTX 3090, which is the literal string
G. That is worth stating because it was filed alongside thelstripbug and the two are three lines apart.BioEngine never sets the accelerator resource; it only reads Ray's. Ray derives the label in
ray/_private/accelerators/nvidia_gpu.pywithagainst the NVML device name. On
"NVIDIA GeForce RTX 3090"the capture group matches uppercase-or-digit characters only, so it stops after theGofGeForceand the node resource really isaccelerator_type:G."NVIDIA A40"and"Tesla T4"capture cleanly, which is why KTH and deNBI look right on the same BioEngine version. I verified the regex is identical in Ray 2.55.1 (the version the workers run) rather than only in the 2.33.0 I happen to have installed locally.The fix direction suggested when this was filed — have the worker pass an explicit
accelerator_type:GeForce-RTX-3090inray start --resources— turns out to be unsound here. Inray/_private/resource_spec.pythe user-supplied resources are copied in first (resources = (self.resources or {}).copy()) and the autodetected constraint is then added unconditionally under its own key. Different key, so the two coexist: the node would advertise bothaccelerator_type:Gandaccelerator_type:GeForce-RTX-3090, and_get_accelerator_typereturns whichever it reaches first while Ray's own@ray.remote(accelerator_type=...)scheduling still sees the wrong one. There is no Ray setting that replaces the autodetected value —RESOURCE_CONSTRAINT_PREFIXis the only accelerator-related constant inray_constants.py.So the honest options are an upstream fix to that regex, or a BioEngine-private resource under a non-reserved key (say
gpu_model:) that the status path prefers. The latter would only cover nodes BioEngine itself starts, leaving KubeRay-managed nodes on Ray's label, and the display is cosmetic. Not doing it here; recorded so the next person does not re-derive it.