WIP: importor skip#2650
Conversation
paul-nechifor
requested review from
Dreamsorcerer,
arkluc,
leshy,
mustafab0 and
spomichter
as code owners
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #2650 +/- ##
==========================================
+ Coverage 71.10% 71.26% +0.15%
==========================================
Files 897 895 -2
Lines 80290 80119 -171
Branches 7183 7192 +9
==========================================
+ Hits 57089 57093 +4
+ Misses 21319 21140 -179
- Partials 1882 1886 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 8 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Greptile SummaryThis PR enforces a no-
Confidence Score: 4/5Safe to merge; all converted imports are backed by verified test-group dependencies, and the new enforcement test correctly self-excludes. Every converted importorskip is either for a package present in the tests dependency group (mujoco, python-can, PyTurboJPEG, opencv-contrib-python via dimos[apriltag]) or is paired with a matching --ignore / collect_ignore guard. The new test_no_importorskip.py codebase check closes the loop cleanly. The one open question is whether adding --ignore to self_hosted_large permanently removes test coverage that should actually run on that runner once native extensions are built there. .github/workflows/ci.yml — the self_hosted_large job now ignores dimos/mapping/ray_tracing and dimos/navigation/nav_3d/mls_planner unconditionally; confirm those native packages are not (and will not be) available on that runner. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[pytest collect] --> B{package available?}
B -->|always in tests group\nmujoco, python-can, cv2.aruco, turbojpeg| C[Direct import at module level\nCollection succeeds]
B -->|native extension\ndimos_mls_planner\ndimos_voxel_ray_tracing| D[CI --ignore flag\nDirectory skipped entirely]
B -->|platform-conditional\nviser on aarch64| E[conftest.py collect_ignore\nFiles excluded on aarch64]
C --> F{marker filter}
F -->|pytestmark = mujoco\nnot in default run| G[Test skipped by marker]
F -->|no restricting marker| H[Test runs]
D --> I[Never collected in CI]
E --> J[Never collected on aarch64]
K[test_no_importorskip.py] -->|fails build if any .py\ncontains importorskip| L[Policy enforced]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[pytest collect] --> B{package available?}
B -->|always in tests group\nmujoco, python-can, cv2.aruco, turbojpeg| C[Direct import at module level\nCollection succeeds]
B -->|native extension\ndimos_mls_planner\ndimos_voxel_ray_tracing| D[CI --ignore flag\nDirectory skipped entirely]
B -->|platform-conditional\nviser on aarch64| E[conftest.py collect_ignore\nFiles excluded on aarch64]
C --> F{marker filter}
F -->|pytestmark = mujoco\nnot in default run| G[Test skipped by marker]
F -->|no restricting marker| H[Test runs]
D --> I[Never collected in CI]
E --> J[Never collected on aarch64]
K[test_no_importorskip.py] -->|fails build if any .py\ncontains importorskip| L[Policy enforced]
Reviews (1): Last reviewed commit: "importor skip" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if _is_ignored_dir(dirpath): | ||
| continue | ||
|
|
||
| for fname in filenames: | ||
| if not fname.endswith(".py"): |
There was a problem hiding this comment.
_is_ignored_dir(dirpath) guard
The dirnames[:] = [d for d in dirnames if d not in IGNORED_DIRS] mutation on the line above already prevents os.walk from ever descending into an ignored directory, so dirpath will never contain a component from IGNORED_DIRS during the traversal. The if _is_ignored_dir(dirpath): continue branch is therefore unreachable for any dirpath below dimos_dir and can be safely removed to avoid misleading readers into thinking there are two independent layers of filtering.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| env: | ||
| DISPLAY: ":0" | ||
| run: uv run pytest --cov=dimos/ --junitxml=junit.xml -m self_hosted_large | ||
| run: uv run pytest --ignore=dimos/mapping/ray_tracing --ignore=dimos/navigation/nav_3d/mls_planner --cov=dimos/ --junitxml=junit.xml -m self_hosted_large |
There was a problem hiding this comment.
--ignore applied to self_hosted_large may permanently suppress coverage
self_hosted_large is the high-memory runner intended for heavier workloads. If dimos_mls_planner or dimos_voxel_ray_tracing are — or ever become — available on that runner (e.g. as a compiled native artifact cached in the runner image), adding the same --ignore flags here means those test directories will never execute there either. A broken change to the ray-tracing or MLS-planner code would not be caught by any CI job. Consider whether the self_hosted_large runner should build and run those suites instead of ignoring them.
|
|
||
| from dimos.constants import DIMOS_PROJECT_ROOT | ||
|
|
||
| PATTERN = "importorskip" |
There was a problem hiding this comment.
PATTERN = "importorskip" matches any occurrence of the string, including lines like # Do not use importorskip here or a docstring explaining the policy. That's probably the desired strictness (zero-tolerance), but a comment explaining the intent would help reviewers who might otherwise try to "work around" the check with a comment. Worth a one-line note in the constant definition.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
I would recommend against this. This will make it more difficult for downstream to test dimos. Instead, I suggest paying more attention to codecov (the CI checks should all be in place now).
For release, we added the error-on-skips to ensure we do test everything before a release (should be an automated step soon), so any damage caused by an importorskip is short term.
As prior art, we use this in several places in aiohttp for similar reasons:
https://github.com/search?q=repo%3Aaio-libs%2Faiohttp%20importorskip&type=code
I also suspect that people will see the importskip being banned in future, and then just use the less preferred try/except pattern to achieve the same result..
2 participants
Problem
Closes DIM-XXX
Solution
How to Test
Contributor License Agreement