Repository navigation
tests: make the MLX boundary guard catch mlx_lm imports - #108
Conversation
shuk777
left a comment
There was a problem hiding this comment.
Thanks for catching this gap. The check should cover mlx_lm as well as mlx. Recording the existing streaming-layer import in an allowlist keeps CI green, and the change is worth taking.
The conflict is from the test file moving. Please don't restore tests/test_repo_hygiene.py at the repo root. Apply it on python/tests/test_repo_hygiene.py instead:
- Use the regex ^\s*(?:from|import)\s+mlx(?:_lm)?\b
- Put python/src/edge0/streaming/layer.py in the allowlist
- Keep the scan on python/src/edge0, and skip allowlisted files
Moving _gather_sort and _scatter_unsort out of mlx_lm would change the streaming path, and it is not required for this PR. Our next step is a unified inference framework across all platforms, planned for November. It is a substantial rework of the current framework, so there is no need to do that move on the current tree. We will use your notes as a reference. Once the new framework is out, we will let you know right away.
Thanks again.
|
Thanks for the review. Updated as requested:
I will not send the |
The regex in test_mlx_imports_stay_inside_the_backend required a space or a dot right after mlx, so from mlx_lm... imports never matched. Tighten it to (from|import)\s+mlx(?:_lm)?\b and list python/src/edge0/streaming/layer.py as a known violation until the helpers move into the framework.
063355b to
82e9f7a
Compare
|
Force-pushed to fix the commit author; the branch is now a single commit on top of current main, with the same content as before. |
shuk777
left a comment
There was a problem hiding this comment.
LGTM, thanks for ur contribute!
What
tests/test_repo_hygiene.py::test_mlx_imports_stay_inside_the_backendenforces the rule stated inbackends/__init__.py: MLX packages are imported only underedge0/backends/mlx/. Its regex requires a space or a dot right aftermlx, sofrom mlx_lm.models... import ...never matches and the guard silently passes framework code that importsmlx_lmdirectly.This PR tightens the pattern to
^\s*(?:from|import)\s+mlx(?:_lm)?\b.What it finds
The stricter guard immediately flags
src/edge0/streaming/layer.py:49:Rather than turn CI red on an existing file, that path is listed in an explicit
_MLX_IMPORT_KNOWN_VIOLATIONSset with a note on what should move into the framework. Remove the entry once the two helpers no longer come frommlx_lm. I'm happy to send that follow-up: they are a few lines of argsort/gather that can live next tomoe/routing.py, which would also make the streaming layer importable on a non-MLX backend.If you would rather have the guard fail loudly instead, drop the allowlist — the regex change alone is the fix.
Checks
pytest tests/test_repo_hygiene.py— 4 passed. The file imports nothing fromedge0ormlx, so this runs on any platform, same as the existing Linux hygiene job.import mlx,import mlx.core as mx,from mlx_lm import x,from mlx_lm.models.base import ..., and does not matchfrom edge0.backends.mlx._impl import ...or a hypotheticalmlxfoopackage.Context
I'm working on a CUDA/PyTorch backend for the reserved
backends/cuda/slot (https://github.com/smartbulldog-pro/edge0-cuda/tree/cuda-backend). This import is the one thing that keepsstreaming/layer.pyfrom importing on a non-MLX backend, which is how I ran into the guard.