Improve error for contrib when the dependency packages are not installed - #3421
Improve error for contrib when the dependency packages are not installed#3421hirosassa wants to merge 7 commits into
Conversation
aea35de to
80900af
Compare
dlstadther
left a comment
dlstadther
left a comment
There was a problem hiding this comment.
I worry a little about the non-dryness between defining the name of the dependent python package(s) as a string in a exception message, when there are a subset of dependency groups defined in the pyproject.toml . A more ideal pattern (IMO) would be to define 1 dependency group per contrib module (where needed) which defines the range of dependent packages and versions it requires. Then the contrib exception can mention something like "missing package dependencies, install with luigi[s3]".
|
|
||
| def __init__(self, *args, **kwargs): | ||
| if not _avro_enabled: | ||
| raise ImportError("avro is required for BigQueryLoadAvro. Install it with: pip install avro-python3") |
There was a problem hiding this comment.
Indirectly related to this PR avro-python3 is deprecated in favor of avro. Something worth addressing separately.
| import pysftp | ||
| except ImportError: | ||
| logger.warning("Please install pysftp to use SFTP.") | ||
| raise ImportError("pysftp is required for SFTP functionality. Install it with: pip install pysftp") |
There was a problem hiding this comment.
Outside of scope - pysftp hasn't been updated in a very long time. Should probably be replaced with paramiko.
02ae457 to
1501bb6
Compare
I agree with this idea. |
|
CI for Azure blob is flaky recently |
dlstadther
left a comment
dlstadther
left a comment
There was a problem hiding this comment.
I'm not sure why the codecov failure is here.
|
@hirosassa , i'm not in a position to override the failing codecov check. It appears the codecov complaint is about the qty of new lines that aren't executed by any test. |
|
@dlstadther Thanks for flagging this. The new lines that codecov is complaining about are the if not _xxx_enabled: raise ImportError(...) checks in each contrib module's init. These paths only execute when the required dependency packages are not installed, but in CI, those dependencies are installed, so these lines are inherently uncoverable under normal test runs. To cover them, we'd need to mock sys.modules or reload modules with dependencies removed, which adds test complexity without much real value — it would just verify that if not flag: raise works. Would it be possible to override the codecov check and merge as-is, or is there a preferred way to handle this? |
Technically, yes. But i don't have the repo permissions to be able to do this. I've historically needed someone like @RRap0so to help in these scenarios. |
2 participants
Description
This PR improves error behavior when using
contribmodules without their required dependencies installed.Motivation and Context
In the current implementation, when a dependency is missing, only a warning is logged at import time. This means users may not notice the issue until the code is actually executed. Furthermore, since the ImportError is silently swallowed in the except block, runtime errors manifest as NameError or AttributeError instead, making root cause investigation difficult.
In this change, I introduce an availability flag in each
contribmodule's try/except ImportError block and checks it in the init of the relevant classes. When a required dependency is not installed, a clear ImportError is raised at instantiation time with a message indicating which package to install (e.g., pip install boto3). This allows users to identify the problem earlier and with a more actionable error message.Have you tested this? If so, how?
Add tests and passed CI successfully.