feat(deprecation): emit DeprecationWarning on import (closes #168) - #173
cagataycali merged 1 commit into
Conversation
…trands-labs#168) The Isaac Sim backend now ships as an in-tree builtin of strands-robots, so strands-robots-sim is being deprecated and archived (epic strands-labs#167). Emit a DeprecationWarning at top-level package import that points users at the concrete migration command `pip install strands-robots[isaac]`, the deprecation epic (strands-labs#167), and examples/MIGRATION.md. Use stacklevel=2 so the warning is attributed to the caller's import line rather than to this module. Also add a `.. deprecated::` note to the package docstring and a pin test (strands_robots_sim/isaac/tests/test_deprecation_warning.py) that asserts a fresh import emits exactly one DeprecationWarning naming the migration command, and that the public surface (__version__) survives the warning.
cagataycali
left a comment
There was a problem hiding this comment.
Summary
The mechanism here is right and the reasoning in the comment block is the kind I want to see kept: stacklevel=2 so python -W all blames the caller's import line rather than this module, the notice placed after __all__ so the public surface is established before anything can warn, and a pin test that checks the public surface still resolves through the warning. The docstring .. deprecated:: note is a nice touch for anyone reading via help().
One blocking issue, and it is the same one as #172 except here it is locked in by a test.
Blocking: the warning names an extra that does not exist, and the test pins it
strands_robots_sim/__init__.py:
_DEPRECATION_MESSAGE = (
"strands-robots-sim is deprecated and will be archived. The Isaac Sim "
"backend now ships as an in-tree builtin of strands-robots; install it "
"with `pip install strands-robots[isaac]`. "
...
)There is no isaac extra on strands-robots. I enumerated [project.optional-dependencies] at strands-labs/robots@main - 26 extras, and the Isaac one is sim-isaac:
groot-service, cosmos3-service, cosmos3-diffusers, cosmos3-sim, moveit2, curobo,
wbc, motionbricks, lerobot, lerobot-async, molmoact2, sim, sim-mujoco,
sim-newton, sim-isaac, sim-gs, benchmark-libero, mesh, mesh-iot,
device-connect, ros2, vera-sim, ollama, inference, all, dev
isaac is absent, and the plain sim extra is not a fallback either - it is sim = ["robot_descriptions>=1.11.0,<2.0.0"] and nothing more.
This matters more than a typo because pip does not fail on an unknown extra. pip install 'strands-robots[isaac]' exits 0, prints one WARNING: strands-robots does not provide the extra 'isaac', and installs the base package with no Isaac dependencies. The user sees a clean install, then hits a missing-usd-core failure at create_simulation("isaac", ...) with nothing connecting it back to the migration step. A deprecation warning whose entire job is to hand over one copy-pasteable command should not hand over one that silently no-ops.
The fix is one token in three places, but note the third:
_DEPRECATION_MESSAGE->pip install strands-robots[sim-isaac]- the module docstring
.. deprecated::note -> same strands_robots_sim/isaac/tests/test_deprecation_warning.py-> the assertion currently pins the wrong string:
assert "pip install strands-robots[isaac]" in message, (
f"... `pip install strands-robots[isaac]`; got: {message!r}"
)So the test does not merely miss the bug, it will fail the moment the message is corrected. Worth calling out because it is the interesting failure mode: the pin test asserts the message is copy-pasteable but never asserts it is correct, and there is no cheap way for a test in this repo to know the set of extras the other repo publishes. If you want the assertion to carry real weight rather than restate the literal, assert on sim-isaac specifically and leave a comment naming robots/pyproject.toml as the source of truth, so the next person renaming an extra has a string to grep for.
Non-blocking
- Default visibility. Per PEP 565
DeprecationWarningis only shown by default when triggered from__main__. A user whose top-level script doesimport strands_robots_simwill see it; a user importing it from inside their own library module will not, unless they run-Wor pytest surfaces it. That is still the correct category for this -DeprecationWarningis what tooling keys on - so I would not change it, but if the goal is that no existing user can miss the handover before the archive, the README banner in #172 is carrying most of that weight and the final-release notes will need to as well. - The comment block explaining
stacklevel=2is genuinely useful; please keep it through any rewrite of the message.
What is good
- Emitting after
__all__is assigned, and pinning that__version__/__all__still resolve through the warning - the failure mode where a warning-on-import shadows an import error is a real one and this rules it out. - The pin test asserts "exactly one" warning on a fresh import, which catches the duplicate-emit-on-reimport class.
- Correctly leaves the legacy-name
__getattr__raisingImportErrorrather than downgrading it to a warning, so-W error::DeprecationWarningenvironments are unaffected by that path. - Scope is tight: two files, no behaviour change beyond the notice itself.
Re-review as soon as the extra name is corrected in the message, the docstring, and the assertion.
What
Emit a
DeprecationWarningat top-level package import (import strands_robots_sim) that points users at the concrete migration commandpip install strands-robots[isaac].Why
Per the deprecation epic (#167), the Isaac Sim backend now ships as an in-tree builtin of
strands-robots, sostrands-robots-simis being deprecated and will be archived. This is the "DeprecationWarning on import" child of that epic (#168). Surfacing the notice at import time gives existing users a runtime signal — before the final release + PyPI deprecation land — with a copy-pasteable migration command.Changes
strands_robots_sim/__init__.pywarnings.warn(..., DeprecationWarning, stacklevel=2)at module load.stacklevel=2attributes the warning to the caller'simportline, not this module, sopython -W all/ pytest-Woutput points at user code.pip install strands-robots[isaac], links the deprecation epic ([EPIC] Deprecate robots-sim (Isaac backend moved to strands-labs/robots) #167), and referencesexamples/MIGRATION.md... deprecated::note to the module docstring.strands_robots_sim/isaac/tests/test_deprecation_warning.py(new pin test)DeprecationWarning.__version__,__all__) survives the warning.Test surface
hatch run lint— clean (black / isort / flake8).hatch run test—267 passed, 40 skipped(skips are GPU/env-gated). The 3 new tests intest_deprecation_warning.pypass.filterwarnings = errorin[tool.pytest.ini_options], and the legacy-name__getattr__guard already raisesImportError(not a warning) so-W error::DeprecationWarningenvs are unaffected.Acceptance criteria (from #168)
DeprecationWarningemitted on package importpip install strands-robots[isaac]Out-of-scope follow-ups (other #167 children, not this PR)
Project board
If this issue is on the Strands Labs - Robots board, please move it to In review (couldn't confirm board membership from the API in this session).
Closes #168.