Python: make optional Lightning observability test skip nested import errors - #8071
Conversation
6246e22 to
9ae04bc
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The focused test-only change has no unresolved issues.
Pull request overview
Updates the optional Lightning observability test to skip when agentlightning or a transitive dependency raises ImportError.
Changes:
- Expands
pytest.importorskipto handle nested import failures. - Documents the optional dependency boundary.
File summaries
| File | Description |
|---|---|
python/packages/lab/lightning/tests/test_lightning.py |
Expands the optional import guard to nested import failures. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Ricky-7-Yan
left a comment
There was a problem hiding this comment.
Reviewed against #8057 and pytest 9.1.1's importorskip implementation. The pinned pytest version now defaults exc_type to ModuleNotFoundError, and exc_type=ImportError is the documented compatibility path for nested import failures. The guard only wraps the optional import, so the tracer setup and assertions still execute whenever agentlightning imports successfully. The change is focused and matches the resource-intensive optional-test boundary; no blocking findings.
9ae04bc to
a84ab0c
Compare
Motivation & Context
The optional Lightning observability test currently uses
pytest.importorskip("agentlightning"). With pytest 9.1, that only skipsModuleNotFoundError; a nestedImportErrorfrom an installed transitive dependency propagates and hard-fails the resource-intensive lab test. This reproduces withagentlightning0.3.0 importing LiteLLM proxy code against FastAPI 0.141, whereget_flat_dependantis no longer available.Description & Review Guide
What are the major changes?
Configure the existing optional dependency guard with
exc_type=ImportError, so both a missingagentlightningpackage and an installed package whose optional dependency import fails are treated as skips.What is the impact of these changes?
The resource-intensive observability test no longer hard-fails CI when an optional transitive import is incompatible. The existing tracer setup and assertions remain unchanged, so the test still runs whenever
agentlightningimports successfully.What do you want reviewers to focus on?
Whether treating all
ImportErrorfailures from the optional integration as a skip matches the intended optional-test boundary.Related Issue
Fixes #8057
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.