Name the installable package in missing dependency errors - #862
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds install-name metadata to optional dependency errors, distinguishes missing modules from internal import failures, formats conditional pip and uv installation guidance, and normalizes dependency reporting in IO scans. ChangesOptional dependency diagnostics
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dascore/utils/misc.py (1)
534-552: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit exception chaining.
Line 552 creates a replacement exception but does not explicitly identify
exas its cause. RaiseMissingOptionalDependencyErrorwithfrom ex. This resolves Ruff B904 and retains the failed import in the traceback.Proposed fix
- raise MissingOptionalDependencyError(msg, install_name=install_name) + raise MissingOptionalDependencyError( + msg, install_name=install_name + ) from ex🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dascore/utils/misc.py` around lines 534 - 552, Update the on_missing == "raise" branch in the ImportError handler to raise MissingOptionalDependencyError with the caught ex explicitly chained as its cause, preserving the existing message and install_name arguments.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dascore/io/core.py`:
- Line 1446: Update the directory-scan path around fiber_io.scan so
MissingOptionalDependencyError is caught and handled like the existing
optional-dependency path, incrementing missing_optional_deps via
_get_missing_install_name(ex) and continuing the scan instead of aborting
dc.scan.
---
Nitpick comments:
In `@dascore/utils/misc.py`:
- Around line 534-552: Update the on_missing == "raise" branch in the
ImportError handler to raise MissingOptionalDependencyError with the caught ex
explicitly chained as its cause, preserving the existing message and
install_name arguments.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0dd6ab42-c5d1-496e-b292-eeb134c2260a
📒 Files selected for processing (7)
dascore/exceptions.pydascore/io/core.pydascore/utils/jit.pydascore/utils/misc.pydocs/changelog.qmdtests/test_io/test_io_core.pytests/test_utils/test_misc.py
| source = fiber_io.scan(resource, **scan_kwargs) | ||
| except MissingOptionalDependencyError as ex: | ||
| missing_optional_deps[ex.msg.split(" ")[0]] += 1 | ||
| missing_optional_deps[_get_missing_install_name(ex)] += 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find directory FiberIO implementations and inspect their scan methods.
ast-grep outline dascore/io --items all --type class,function
rg -n -P -C 8 --glob '*.py' 'input_type\s*=\s*["'\'']directory["'\'']|def\s+scan\s*\(' dascore/io
# Find directory scan paths that use optional imports or raise missing-dependency errors.
rg -n -P -C 6 --glob '*.py' '\boptional_import\s*\(|\bMissingOptionalDependencyError\b' dascore/ioRepository: DASDAE/dascore
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- scan dispatch ---'
cat -n dascore/io/core.py | sed -n '1385,1460p'
printf '%s\n' '--- directory FiberIO classes ---'
python3 - <<'PY'
import ast
from pathlib import Path
for path in Path("dascore/io").rglob("*.py"):
try:
tree = ast.parse(path.read_text())
except Exception:
continue
for node in ast.walk(tree):
if not isinstance(node, ast.ClassDef):
continue
input_type = None
bases = []
for base in node.bases:
if isinstance(base, ast.Name):
bases.append(base.id)
elif isinstance(base, ast.Attribute):
bases.append(base.attr)
for stmt in node.body:
if isinstance(stmt, ast.Assign):
for target in stmt.targets:
if isinstance(target, ast.Name) and target.id == "input_type":
if isinstance(stmt.value, ast.Constant):
input_type = stmt.value.value
if input_type == "directory":
scan = next(
(x for x in node.body if isinstance(x, (ast.FunctionDef, ast.AsyncFunctionDef))
and x.name == "scan"),
None,
)
print(path, node.name, "bases=", bases,
"scan_lines=", (scan.lineno, scan.end_lineno) if scan else None)
PY
printf '%s\n' '--- XMLBinary imports and scan helpers ---'
rg -n -C 8 --glob '*.py' \
'optional_import|MissingOptionalDependencyError|def (_read_xml_metadata|_paths_to_scan_patches|scan)' \
dascore/io/xml_binary dascore/core dascore/utils.py dascore/exceptions.py 2>/dev/null || true
printf '%s\n' '--- all directory declarations and exception raises ---'
rg -n -C 4 --glob '*.py' \
'input_type\s*=\s*["'\'']directory["'\'']|raise\s+MissingOptionalDependencyError|MissingOptionalDependencyError\s*=' \
dascoreRepository: DASDAE/dascore
Length of output: 13842
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- scan dispatch ---'
cat -n dascore/io/core.py | sed -n '1385,1460p'
printf '%s\n' '--- directory FiberIO classes ---'
python3 - <<'PY'
import ast
from pathlib import Path
for path in Path("dascore/io").rglob("*.py"):
tree = ast.parse(path.read_text())
for node in ast.walk(tree):
if not isinstance(node, ast.ClassDef):
continue
input_type = None
bases = []
for base in node.bases:
bases.append(base.id if isinstance(base, ast.Name) else getattr(base, "attr", None))
for stmt in node.body:
if isinstance(stmt, ast.Assign):
for target in stmt.targets:
if isinstance(target, ast.Name) and target.id == "input_type":
if isinstance(stmt.value, ast.Constant):
input_type = stmt.value.value
if input_type == "directory":
scan = next(
(x for x in node.body if isinstance(x, ast.FunctionDef) and x.name == "scan"),
None,
)
print(path, node.name, bases,
(scan.lineno, scan.end_lineno) if scan else None)
PY
printf '%s\n' '--- XMLBinary dependency paths ---'
rg -n -C 8 --glob '*.py' \
'optional_import|MissingOptionalDependencyError|def (_read_xml_metadata|_paths_to_scan_patches|scan)' \
dascore/io/xml_binary dascore 2>/dev/null | head -300
printf '%s\n' '--- directory declarations and dependency exceptions ---'
rg -n -C 4 --glob '*.py' \
'input_type\s*=\s*["'\'']directory["'\'']|raise\s+MissingOptionalDependencyError|MissingOptionalDependencyError\s*=' \
dascoreRepository: DASDAE/dascore
Length of output: 23056
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- XMLBinary modules ---'
cat -n dascore/io/xml_binary/core.py | sed -n '1,90p'
cat -n dascore/io/xml_binary/utils.py | sed -n '1,270p'
printf '%s\n' '--- XML conversion definition and imports ---'
rg -n -C 8 --glob '*.py' \
'xml_to_dict|def\s+xml_to_dict|from .*xml|import .*xml|optional_import' \
dascore
printf '%s\n' '--- FiberIO registry and directory resolution ---'
rg -n -C 8 --glob '*.py' \
'register|FIBER|fiber_io|input_type|is_directory_format|_get_fiber_io_and_req_type' \
dascore/io/__init__.py dascore/io/core.py dascore 2>/dev/null | head -500Repository: DASDAE/dascore
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- io package registration ---'
wc -l dascore/io/__init__.py
cat -n dascore/io/__init__.py | sed -n '1,240p'
printf '%s\n' '--- manager and FiberIO lookup definitions ---'
ast-grep outline dascore/io/core.py --items all --type class,function | sed -n '1,180p'
rg -n -C 12 \
'class _FiberIOManager|class FiberIO|def _get_fiber_io_and_req_type|fiber_io_classes|FIBER_IO|entry_points|importlib.metadata|register' \
dascore/io/core.py dascore/io/__init__.pyRepository: DASDAE/dascore
Length of output: 18299
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
tree = ast.parse(Path("dascore/io/core.py").read_text())
iter_scan = next(
node for node in ast.walk(tree)
if isinstance(node, ast.FunctionDef) and node.name == "_iter_scan_results"
)
directory_if = next(
node for node in ast.walk(iter_scan)
if isinstance(node, ast.If)
and isinstance(node.test, ast.Compare)
and any(
isinstance(part, ast.Constant) and part.value == "directory"
for part in [node.test.left, *node.test.comparators]
)
)
directory_scan = next(
node for node in ast.walk(directory_if)
if isinstance(node, ast.Assign)
and isinstance(node.value, ast.Call)
and isinstance(node.value.func, ast.Attribute)
and node.value.func.attr == "scan"
)
dependency_handler = next(
node for node in ast.walk(iter_scan)
if isinstance(node, ast.ExceptHandler)
and isinstance(node.type, ast.Name)
and node.type.id == "MissingOptionalDependencyError"
)
non_directory_try = next(
node for node in ast.walk(iter_scan)
if isinstance(node, ast.Try)
and dependency_handler in node.handlers
)
def contains(node, target):
return any(child is target for child in ast.walk(node))
print("directory_scan_line:", directory_scan.lineno)
print("directory_scan_inside_dependency_try:", contains(non_directory_try, directory_scan))
print("plugin_entry_point:", "dascore.fiber_io" in Path("dascore/io/core.py").read_text())
print("registers_by_input_type:", any(
isinstance(node, ast.Call)
and isinstance(node.func, ast.Attribute)
and node.func.attr == "setdefault"
and any(isinstance(arg, ast.Attribute) and arg.attr == "input_type" for arg in node.args)
for node in ast.walk(tree)
))
PYRepository: DASDAE/dascore
Length of output: 277
Handle missing optional dependencies for directory scans.
fiber_io.scan at line 1438 runs outside the MissingOptionalDependencyError handler. A directory FiberIO plugin that requires an optional package can abort dc.scan instead of aggregating the dependency and continuing. Add equivalent handling for directory scans.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dascore/io/core.py` at line 1446, Update the directory-scan path around
fiber_io.scan so MissingOptionalDependencyError is caught and handled like the
existing optional-dependency path, incrementing missing_optional_deps via
_get_missing_install_name(ex) and continuing the scan instead of aborting
dc.scan.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #862 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 176 176
Lines 19276 19313 +37
=========================================
+ Hits 19276 19313 +37
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e06c83d2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not (failed := error.name or ""): | ||
| return False | ||
| return import_name == failed or import_name.startswith(f"{failed}.") |
There was a problem hiding this comment.
When an installed submodule executes something like from package import missing_symbol, Python raises an ImportError whose name is package; this prefix check therefore returns true for optional_import("package.submodule") even though the requested module exists. The caller is consequently told to reinstall package, contrary to the intended handling of failures inside installed packages. Account for import-from errors, such as by checking the exception type and path, rather than treating every named parent as missing.
Useful? React with 👍 / 👎.
| except MissingOptionalDependencyError as ex: | ||
| missing_optional_deps[ex.msg.split(" ")[0]] += 1 | ||
| missing_optional_deps[_get_missing_install_name(ex)] += 1 | ||
| continue |
There was a problem hiding this comment.
When optional_import correctly identifies an ImportError from inside an installed package, it raises MissingOptionalDependencyError with both install_name and name unset. This aggregation then stores the failure under an empty key and discards its original diagnostic, so dc.scan eventually reports unknown (...) and still claims that additional packages need installing instead of showing the actionable could not be imported (<original error>) message. Preserve these unidentifiable exceptions separately or surface their original diagnostic rather than aggregating them as missing installs.
Useful? React with 👍 / 👎.
Optional dependency errors reported the import name, which is not always
something that can be installed. Scanning a directory of Sintela protobuf
files reported {'google.protobuf.descriptor_pb2': 10308}, leaving no
indication that protobuf is the package to install.
Missing dependency messages now name the package and give the pip and uv
commands to install it, and MissingOptionalDependencyError carries the
install name so scan can aggregate on it.
An ImportError raised inside an installed package is no longer reported
as a missing install, since installing the package again wouldn't help.
Consolidate the install name tests down to what covers the behavior, and make the failed import the explicit cause of the raised error.
4e06c83 to
0a4cf24
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_utils/test_misc.py (1)
465-486: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the full internal-import failure contract.
This test exercises only
on_missing="raise"and verifies the source errors only throughstr(error). Add anon_missing="ignore"case that returnsNone, and assert thatMissingOptionalDependencyErrorretains the correspondingModuleNotFoundErrororImportErrorthrough its public cause or attribute. A regression in either contract can otherwise pass this test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_utils/test_misc.py` around lines 465 - 486, Extend test_broken_install_gives_no_install_advice to cover both on_missing="raise" and on_missing="ignore": assert the ignore path returns None, and for the raise path verify MissingOptionalDependencyError preserves the original ModuleNotFoundError or ImportError via its public cause or error attribute, not only through the rendered message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_utils/test_misc.py`:
- Around line 465-486: Extend test_broken_install_gives_no_install_advice to
cover both on_missing="raise" and on_missing="ignore": assert the ignore path
returns None, and for the raise path verify MissingOptionalDependencyError
preserves the original ModuleNotFoundError or ImportError via its public cause
or error attribute, not only through the rendered message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ceb9130-75f6-4ae4-94a5-d7cb8e01d903
📒 Files selected for processing (3)
dascore/utils/misc.pytests/test_io/test_io_core.pytests/test_utils/test_misc.py
🚧 Files skipped from review as they are similar to previous changes (2)
- dascore/utils/misc.py
- tests/test_io/test_io_core.py
coderabbitai
Bot
added
documentation
|
✅ Documentation built: |
1 participant
Description
Missing optional dependency messages reported the import name, which is not always something you can install. Scanning a directory of Sintela protobuf files reported:
Helpful that it found them, but nothing there says the package to install is
protobuf.Implementation notes:
_INSTALL_NAMESindascore.utils.miscmaps import names which differ from their distribution (google.protobuf->protobuf,yaml->pyyaml); submodules resolve to their parent._get_install_messagerenders the pip/uv commands and is also used byoptional_importand the numba JIT error.MissingOptionalDependencyErrorcarries aninstall_name, sodc.scanaggregates on the installable name rather than re-parsing the message. It defaults on the class so subclasses which skip the init still have it."<module> is not installed"message form, so an arbitrary message from a third party FiberIO can't turn intopip install <first word>.User-facing changes
protobuf (10308 files)andInstall with `pip install protobuf` or `uv pip install protobuf`instead of the import namegoogle.protobuf.descriptor_pb2, which is not something you can install.ImportErrorraised inside an installed package is no longer reported as a missing install, since installing the package again wouldn't help. Those reportcould not be imported (<original error>)with the original error as the cause, and no install advice.Breaking changes
None.
Changelog
MissingOptionalDependencyErrorcarries aninstall_name. AnImportErrorraised inside an installed package is reported as a failed import rather than a missing install.Checklist
I have (if applicable):
Summary by CodeRabbit