Add import check for optional dependency on pyiceberg_core #2221
Conversation
Added a NotInstalledException check when pyiceberg_core is imported for pyarrow_transforms This will raise a helpful error message when endusers try to use methods that depend on pyiceberg_core but haven't installed the optional dependency
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM! Thanks for adding this. I added a few nit comments
btw we added pyiceberg_core as a dep of pyarrow extra
Line 298 in ad8263b
pyiceberg/transforms.py
Outdated
| TRUNCATE_PARSER = ParseNumberFromBrackets(TRUNCATE) | ||
|
|
||
|
|
||
| def _try_import(module_name: str, extras_name: str | None = None) -> types.ModuleType: |
There was a problem hiding this comment.
nit: feels like we should move this somewhere else, other than in transforms.py but i cant find a suitable place
There was a problem hiding this comment.
Yeah, I couldn't immediately find a spot for it - I saw some other uses of an import check in tables which could potentially also use the function.
Could potentially go in utils/optional_deps.py or something similar? Given the number of optional dependencies, I'm sure there are other areas that could use a helper like this
Pandas has it under compat/_optional.py for reference: https://github.com/pandas-dev/pandas/blob/main/pandas/compat/_optional.py
There was a problem hiding this comment.
i think its fine to leave this here now :)
|
looks like linter errored, could you run we still support python 3.9 which dont have the -> |
|
thanks for the pr @andersbogsnes |
|
Thanks for the review! Hope to do some more now that I'm set up 😄 |
Added a NotInstalledException check when pyiceberg_core is imported for pyarrow_transforms This will raise a helpful error message when endusers try to use methods that depend on pyiceberg_core but haven't installed the optional dependency <!-- Thanks for opening a pull request! --> Closes apache#1987 # Rationale for this change If an enduser hasn't installed the `pyiceberg_core` optional dependency, using pyarrow transforms will crash with an unhelpful error. This PR gives the enduser a nicer error message that informs them to install the optional dependency # Are these changes tested? Yes, a test was added to verify the behaviour # Are there any user-facing changes? <!-- In the case of user-facing changes, please add the changelog label. --> --------- Co-authored-by: Anders Bogsnes <andersbognes@gmail.com>
2 participants
Added a NotInstalledException check when pyiceberg_core is imported for pyarrow_transforms This will raise a helpful error message when endusers try to use methods that depend on pyiceberg_core but haven't installed the optional dependency
Closes #1987
Rationale for this change
If an enduser hasn't installed the
pyiceberg_coreoptional dependency, using pyarrow transforms will crash with an unhelpful error. This PR gives the enduser a nicer error message that informs them to install the optional dependencyAre these changes tested?
Yes, a test was added to verify the behaviour
Are there any user-facing changes?