fix: correct false import suggestions by GopalGB · Pull Request #1788 · pydantic/logfire

GopalGB · 2026-03-21T05:38:20Z

Summary

Fixes #1296 — logfire run always suggested requests, sqlite3, and urllib for instrumentation, even for a simple print('Hello, world!') script.

Root cause:

sqlite3 and urllib were always recommended because they were in STANDARD_LIBRARY_PACKAGES, which bypassed the "is this package installed?" check
requests was recommended because it's a transitive dependency of opentelemetry, so it appeared as "installed" even though the user's code doesn't use it

Fix:

Added find_script_imports() which uses Python's ast module to extract top-level imports from the user's script/module
Renamed STANDARD_LIBRARY_PACKAGES to ALWAYS_AVAILABLE_PACKAGES and added requests to it
For packages in ALWAYS_AVAILABLE_PACKAGES, only recommend them if the user's code actually imports them
When no script/module context is available (e.g. logfire inspect), behavior is unchanged — all available packages are still recommended

Before:

$ logfire run script.py  # script.py: print('Hello, world!')
☐ requests (need to install opentelemetry-instrumentation-requests)
☐ sqlite3 (need to install opentelemetry-instrumentation-sqlite3)
☐ urllib (need to install opentelemetry-instrumentation-urllib)

After:

$ logfire run script.py  # script.py: print('Hello, world!')
# No false suggestions

Test plan

Existing tests pass (backward compatible — find_recommended_instrumentations_to_install without script_imports param behaves identically)
logfire run script.py with print('Hello') shows no suggestions for requests/sqlite3/urllib
logfire run script.py with import sqlite3 correctly suggests sqlite3 instrumentation
logfire inspect still shows all available recommendations (no script context = no filtering)
logfire run -m module_name correctly analyzes module imports

🤖 Generated with Claude Code

Summary by cubic

Fixes #1296 by stopping false import suggestions in logfire run. We now analyze the script or module entrypoint imports, so requests, sqlite3, and urllib are only suggested when used.

Bug Fixes
- Added find_script_imports() (AST) to read imports from a script or -m module; prefers __main__.py over __init__.py and skips filtering if source can’t be read.
- Replaced STANDARD_LIBRARY_PACKAGES with ALWAYS_AVAILABLE_PACKAGES (adds requests); only recommend these if actually imported by the user’s code.
- Moved sys.path.insert(0, os.getcwd()) before import analysis so local -m modules resolve correctly.
- Threaded script context through: collect_instrumentation_context() and find_recommended_instrumentations_to_install() now accept script_imports; behavior unchanged when no script/module is provided.

^{Written for commit 3dcf344. Summary will update on new commits.}

`logfire run` was always suggesting `requests`, `sqlite3`, and `urllib` for instrumentation, even when the user's script didn't use them. - `sqlite3` and `urllib` were always suggested because they were in `STANDARD_LIBRARY_PACKAGES` which bypassed the installed-check - `requests` was suggested because it's a transitive dependency of opentelemetry, so it appeared as "installed" The fix introduces AST-based import analysis of the user's script to detect which packages are actually used. Packages in the new `ALWAYS_AVAILABLE_PACKAGES` set (stdlib modules and known transitive deps) are only recommended when the script actually imports them. When no script/module is provided (e.g. `logfire inspect`), the behavior is unchanged -- all available packages are still recommended. Fixes pydantic#1296 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

codecov · 2026-03-21T05:45:03Z

Codecov Report

❌ Patch coverage is 71.42857% with 16 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
logfire/_internal/cli/run.py	71.42%	12 Missing and 4 partials ⚠️

📢 Thoughts on this report? Let us know!

devin-ai-integration

Devin Review found 3 potential issues.

View 2 additional findings in Devin Review.

devin-ai-integration · 2026-03-21T05:45:56Z

logfire/_internal/cli/run.py

+        elif isinstance(node, ast.ImportFrom):
+            if node.module:
+                imports.add(node.module.split('.')[0])


🟡 Relative imports incorrectly treated as absolute imports in find_script_imports

The find_script_imports function does not check node.level on ast.ImportFrom nodes, so relative imports like from .requests import foo (where node.module='requests' and node.level=1) are incorrectly treated as absolute imports of the requests package. This causes the package to be added to the script_imports set, which in turn causes find_recommended_instrumentations_to_install at logfire/_internal/cli/run.py:212-216 to recommend instrumentation for a package the user isn't actually importing. The fix is to additionally check node.level == 0 before adding the module name.

Suggested change

elif isinstance(node, ast.ImportFrom):

if node.module:

imports.add(node.module.split('.')[0])

elif isinstance(node, ast.ImportFrom):

if node.module and node.level == 0:

imports.add(node.module.split('.')[0])

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-03-21T05:45:58Z

logfire/_internal/cli/run.py

+    elif module_name:
+        try:
+            spec = importlib.util.find_spec(module_name)
+            if spec and spec.origin:
+                with open(spec.origin) as f:
+                    source = f.read()
+        except (ModuleNotFoundError, ValueError, OSError):
+            return None


🚩 Module mode analyzes framework source, not user application code

When using logfire run -m uvicorn main:app, the code passes module_name='uvicorn' to find_script_imports (logfire/_internal/cli/run.py:96), which calls importlib.util.find_spec('uvicorn'). This analyzes the framework's source code rather than the user's application (main.py). In practice, for most framework packages (like uvicorn), find_spec returns origin=None (since they're packages, not single-file modules), so find_script_imports correctly falls back to returning None and no filtering is applied. However, if a framework is a single-file module with an origin, the wrong file would be analyzed, potentially filtering out valid recommendations. This is a design limitation more than a bug, but worth being aware of.

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-03-21T05:45:59Z

logfire/_internal/cli/run.py

+def find_script_imports(script_path: str | None = None, module_name: str | None = None) -> set[str] | None:
+    """Extract top-level import names from a script file or module.
+
+    Uses AST parsing to find all `import X` and `from X import ...` statements,
+    returning the set of top-level module names (e.g. 'requests', 'sqlite3').
+
+    Returns None if the source cannot be determined (e.g. no script or module provided),
+    which signals that filtering should be skipped (all packages recommended as before).
+    """
+    source: str | None = None
+
+    if script_path:
+        try:
+            with open(script_path) as f:
+                source = f.read()
+        except OSError:
+            return None
+    elif module_name:
+        try:
+            spec = importlib.util.find_spec(module_name)
+            if spec and spec.origin:
+                with open(spec.origin) as f:
+                    source = f.read()
+        except (ModuleNotFoundError, ValueError, OSError):
+            return None
+
+    if source is None:
+        return None
+
+    try:
+        tree = ast.parse(source)
+    except SyntaxError:
+        return None
+
+    imports: set[str] = set()
+    for node in ast.walk(tree):
+        if isinstance(node, ast.Import):
+            for alias in node.names:
+                # 'import urllib.request' -> top-level is 'urllib'
+                imports.add(alias.name.split('.')[0])
+        elif isinstance(node, ast.ImportFrom):
+            if node.module:
+                imports.add(node.module.split('.')[0])
+    return imports


🚩 No new tests for the script_imports filtering feature

The existing test at tests/test_cli.py:353 calls find_recommended_instrumentations_to_install with only 3 arguments, so script_imports defaults to None. There are no tests verifying the behavior when script_imports is a non-None set (the core new functionality). There are also no tests for find_script_imports itself. This means the new filtering logic, relative import handling, and module resolution paths are all untested.

Was this helpful? React with 👍 or 👎 to provide feedback.

1. Move sys.path.insert(0, os.getcwd()) BEFORE collect_instrumentation_context() so that local modules (-m myapp) can be resolved by importlib.util.find_spec. Previously, find_spec ran before cwd was on sys.path, causing silent fallback to unfiltered recommendations. 2. For packages, analyze __main__.py instead of __init__.py when it exists. runpy.run_module() executes __main__.py at runtime, so import analysis should match the actual entrypoint to produce accurate recommendations.

GopalGB · 2026-03-21T07:29:01Z

Thanks for the thorough review @cubic-dev-ai! Both issues were valid — pushed fixes in 3dcf344:

Fix 1 (P2: sys.path ordering): Moved sys.path.insert(0, os.getcwd()) before collect_instrumentation_context() so that -m local modules can be resolved by find_spec during import analysis.

Fix 2 (P2: init.py vs main.py): Added logic to prefer __main__.py over __init__.py when analyzing packages, since runpy.run_module() executes __main__.py at runtime.

Both changes preserve backward compatibility — script_imports=None still skips filtering for logfire inspect.

cubic-dev-ai

1 issue found across 1 file (changes from recent commits).

Prompt for AI agents (unresolved issues)


Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="logfire/_internal/cli/run.py">

<violation number="1" location="logfire/_internal/cli/run.py:98">
P1: CWD is prepended to `sys.path` too early, allowing local module shadowing during instrumentation imports.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

cubic-dev-ai · 2026-03-21T07:32:38Z

logfire/_internal/cli/run.py

+
+    # Add the current directory to `sys.path` BEFORE import analysis so that
+    # local modules (e.g. `logfire run -m myapp`) can be resolved by find_spec.
+    sys.path.insert(0, os.getcwd())


P1: CWD is prepended to sys.path too early, allowing local module shadowing during instrumentation imports.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At logfire/_internal/cli/run.py, line 98: <comment>CWD is prepended to `sys.path` too early, allowing local module shadowing during instrumentation imports.</comment> <file context> @@ -93,6 +93,10 @@ def parse_run(args: argparse.Namespace) -> None: + # Add the current directory to `sys.path` BEFORE import analysis so that + # local modules (e.g. `logfire run -m myapp`) can be resolved by find_spec. + sys.path.insert(0, os.getcwd()) + ctx = collect_instrumentation_context(exclude, script_path=script_path, module_name=module_name) </file context>

This comment was marked as resolved.

Sign in to view

devin-ai-integration bot reviewed Mar 21, 2026

View reviewed changes

cubic-dev-ai bot reviewed Mar 21, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: correct false import suggestions#1788

fix: correct false import suggestions#1788
GopalGB wants to merge 2 commits intopydantic:mainfrom
GopalGB:fix/false-import-suggestions-1296

GopalGB commented Mar 21, 2026 •

edited by cubic-dev-ai bot

Loading

Uh oh!

This comment was marked as resolved.

Uh oh!

codecov bot commented Mar 21, 2026 •

edited

Loading

Uh oh!

devin-ai-integration bot left a comment

Uh oh!

devin-ai-integration bot Mar 21, 2026

Uh oh!

devin-ai-integration bot Mar 21, 2026 •

edited

Loading

Uh oh!

devin-ai-integration bot Mar 21, 2026 •

edited

Loading

Uh oh!

GopalGB commented Mar 21, 2026

Uh oh!

cubic-dev-ai bot left a comment

Uh oh!

cubic-dev-ai bot Mar 21, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

GopalGB commented Mar 21, 2026 • edited by cubic-dev-ai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by cubic

Uh oh!

This comment was marked as resolved.

Uh oh!

codecov bot commented Mar 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration bot Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration bot Mar 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration bot Mar 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GopalGB commented Mar 21, 2026

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai bot Mar 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

GopalGB commented Mar 21, 2026 •

edited by cubic-dev-ai bot

Loading

codecov bot commented Mar 21, 2026 •

edited

Loading

devin-ai-integration bot Mar 21, 2026 •

edited

Loading

devin-ai-integration bot Mar 21, 2026 •

edited

Loading

cubic-dev-ai bot Mar 21, 2026 •

edited

Loading