Fix Pink IK DAQP dependency checks#5556
Conversation
Greptile SummaryThis PR fixes silent failures in the Pink IK controller by adding an explicit DAQP solver availability check at construction time and expanding the install-time probe to verify the full dependency stack (
Confidence Score: 4/5Safe to merge — the changes add early-failure guards that eliminate a previously confusing silent fallback, with no risk to existing working environments. The install probe and controller constructor check are both correct and cover the described failure mode. The only noted concern is a dead-code ImportError handler inside _validate_solver_available that can never fire given the module-level pink import, but this does not affect runtime behavior. No files require special attention; pink_ik.py has the minor dead-code note but is otherwise clean. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[isaaclab.sh -i / command_install] --> B[_ensure_pink_ik_dependencies_installed]
B --> C{Linux x86_64/aarch64?}
C -- No --> D[Skip]
C -- Yes --> E["Probe: import pinocchio, daqp, qpsolvers\nassert 'daqp' in available_solvers"]
E -- returncode == 0 --> F[Dependencies OK]
E -- returncode != 0 --> G[Force-reinstall pin + pin-pink==3.1.0 + daqp==0.7.2]
G -- success --> F
G -- failure --> H[print_warning — install continues]
I[PinkIKController.__init__] --> J[_validate_consistency]
J --> K[_validate_solver_available]
K --> L{qpsolvers importable?}
L -- No --> M[RuntimeError: install stack]
L -- Yes --> N{"'daqp' in available_solvers?"}
N -- No --> O[RuntimeError: solver missing]
N -- Yes --> P[Continue initialization]
P --> Q[compute: solve_ik with solver=daqp]
Reviews (1): Last reviewed commit: "Fix Pink IK DAQP dependency checks" | Re-trigger Greptile |
| try: | ||
| from qpsolvers import available_solvers | ||
| except ImportError as exc: | ||
| raise RuntimeError( | ||
| "Pink IK requires the qpsolvers package. Install the Pink IK stack with " | ||
| "``./isaaclab.sh -i`` or manually install ``pin pin-pink==3.1.0 daqp==0.7.2``." | ||
| ) from exc |
There was a problem hiding this comment.
Unreachable
ImportError guard on qpsolvers
The try/except ImportError block around from qpsolvers import available_solvers can never trigger: this file already imports from pink import solve_ik at module scope, and qpsolvers is a hard dependency of pin-pink. If qpsolvers is absent, the module-level import fails before _validate_solver_available is ever called. The defensive guard is dead code. Consider removing it and letting the module-level import provide the natural failure signal, or add a comment explaining the defensive intent.
There was a problem hiding this comment.
Review Summary
Clean, well-scoped bug fix. The problem (DAQP missing → silent IK failure) is diagnosed correctly, and the fix addresses both runtime validation and install-time probing.
Strengths
- Proper fail-fast with clear error messages pointing to the exact fix command
- Install probe expanded to validate the full dependency chain, not just
import pinocchio - Good naming improvements (
_PINOCCHIO_STACK→_PINK_IK_STACK, function rename) _QP_SOLVERconstant eliminates magic string duplication
Minor Suggestions
See inline comments.
|
|
||
| if _QP_SOLVER not in available_solvers: | ||
| raise RuntimeError( | ||
| f"Pink IK requires the '{_QP_SOLVER}' QP solver, but qpsolvers reports available solvers: " |
There was a problem hiding this comment.
💡 Consider making the solver name configurable: _QP_SOLVER is a module-level constant. If a user installs a different QP solver (e.g., osqp, proxqp) they'd have to monkey-patch this. Consider making it a field on PinkIKControllerCfg with a default of "daqp", so validation stays but users get a knob:
# In PinkIKControllerCfg:
qp_solver: str = "daqp"This is a future enhancement suggestion, not a blocker for this fix.
|
|
||
| probe_result = run_command( | ||
| [python_exe, "-c", "import pinocchio"], | ||
| [ |
There was a problem hiding this comment.
🟡 Probe assertion may fail on older qpsolvers versions: The probe runs assert 'daqp' in qpsolvers.available_solvers. If qpsolvers is installed but at a version where available_solvers has a different type or naming scheme, this assertion could fail even if daqp is usable. Consider catching the assertion error in the probe gracefully (the probe already handles non-zero returncode, so this is fine as-is, but worth noting).
Keep the installer responsible for checking the Pink IK dependency stack. At runtime, only surface a clear failure when qpsolvers reports the configured DAQP solver is missing, while preserving the existing IK fallback path for ordinary solver failures.
Require a DAQP build that supports qpsolvers warm-start arguments and make the installer probe for that API shape. Surface the specific primal_start mismatch at runtime instead of treating it as an ordinary IK fallback.
Pink IK imports qpsolvers for the runtime solver error path, but the docs environment can build API pages without the optional solver stack. Add qpsolvers to the existing Sphinx mocked optional dependencies so PinkIKController autodoc imports cleanly.
Description
Pink IK uses DAQP through
qpsolvers, but the install-time dependency repair onlyverified that Pinocchio could be imported. In environments where
pin-pinkorqpsolversis present but DAQP is missing, unregistered, or too old forqpsolverswarm-start arguments,solve_ik(..., solver="daqp")can fail andfall back to current joint targets. The controller then reports a misleading
end-effector position error in
test_pink_ik.py.This change makes the installer probe the full Pink IK stack:
pinocchio, DAQPregistration in
qpsolvers, and thedaqp.solveAPI shape required by currentqpsolvers(primal_start). It also aligns the IsaacLab dependency pin withthe compatible DAQP release,
daqp==0.8.5.Runtime handling stays narrow: ordinary IK failures keep the existing fallback,
while missing DAQP or the specific
primal_startAPI mismatch is surfaced withan actionable install message instead of being swallowed as an IK fallback.
The docs config also mocks
qpsolvers, matching the existing docs treatment foroptional Pink IK dependencies such as
pinkandpinocchio, so API docs can bebuilt without the runtime solver stack installed.
Type of change
Test Plan
./isaaclab.sh -p -m py_compile source/isaaclab/isaaclab/cli/commands/install.py source/isaaclab/isaaclab/controllers/pink_ik/pink_ik.py source/isaaclab/setup.py docs/conf.py./isaaclab.sh -p -c "import inspect, pinocchio, daqp, qpsolvers; assert 'daqp' in qpsolvers.available_solvers; assert 'primal_start' in inspect.signature(daqp.solve).parameters; print('pink ik dependency probe passed')"./isaaclab.sh -p -c "..."small monkeypatch check thatTypeError("solve() got an unexpected keyword argument 'primal_start'")raises the new DAQP compatibilityRuntimeError./isaaclab.sh -p -m pytest source/isaaclab/test/controllers/test_pink_ik.py::test_movement_types -k "GR1T2-Abs-v0 and stay_still" -q --tb=short -s -x./isaaclab.sh -p -m pytest source/isaaclab/test/controllers/test_pink_ik.py -q --tb=short -x(23 passed, 1 skipped)VIRTUAL_ENV=/home/zhengyuz/Projects/IsaacLab.wt/feature-heterogeneous_dexsuite/env_isaaclab PATH=/home/zhengyuz/Projects/IsaacLab.wt/feature-heterogeneous_dexsuite/env_isaaclab/bin:$PATH make current-docsfromdocs/VIRTUAL_ENV=/home/zhengyuz/Projects/IsaacLab.wt/feature-heterogeneous_dexsuite/env_isaaclab ./isaaclab.sh -fScreenshots
N/A.
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml-- CI handles that)CONTRIBUTORS.mdor my name already exists there