Refactor dependencies to make more optional#1548
Conversation
Related to #1224; addresses user feedback asking to handle dependencies in a more elegant manner. Many previously-required dependencies are now optional. Also may speed up "import uxarray" slightly, as now-optional dependencies are moved to no longer be at top of files; instead, they are imported the first time any relevant method gets called. See upcoming associated PR for more notes / reasoning / discussion.
(copilot suggestions hallucinated a strange import, this commit fixes that.)
Missed an import of GeoAxes, while refactoring to remove required dependencies. Added pooch dependency, which is necessary for test/io/test_structure.py tests to run, and previously was not included anywhere in dependencies.
because #1541 was merged!
also clarifies: conda installation automatically includes optional dependencies, not just required dependencies.
|
One thing to flag: anyone doing |
(forgot to remove the comment when moving hvplot dependency during cb8c8a3...)
I definitely agree, we should make sure to discuss more before merging. I am curious to know what people think. One thought I had is: something that might help could be to provide hints during those crashes. Something like: def function_that_uses_holoviews(*args, **kwargs):
try:
import holoviews as hv
except ImportError as err:
raise ImportError('holoviews import failed; to install all plotting dependencies, could do: pip install "uxarray[viz]".') from err
# << code from functionAs opposed to the code presently in the PR, which just does Although, this doesn't match xarray's style. In xarray, if you fail to install optional dependencies you will just see an ImportError crash once you try to use them, there's no hint to pip install xarray[viz] or anything like that. Another slight downside is that this option might get a bit unwieldy due to the need to repeat it inside every relevant method. A partial workaround could be to handle these hints all in one place: # --- inside new file: utils.imports.py --- #
def _import_with_hint_if_error(*modules):
"""import modules, but make nice error if crash, providing hint to pip install "uxarray[...]".
E.g., if 'holoviews' in modules, and import holoviews fails, hint to use: pip install "uxarray[viz]".
"""
if 'holoviews' in modules:
try:
import holoviews as hv
except ImportError as err:
raise ImportError('holoviews import failed; to install all plotting dependencies, could do: pip install "uxarray[viz]".') from err
if 'geoviews' in modules:
# similar
# similar for all other optional dependencies.Then functions with optional dependencies can look like: from uxarray.utils.imports import _import_with_hint_if_error
def function_that_uses_holoviews_and_geoviews(*args, **kwargs):
_import_with_hint_if_error('holoviews', 'geoviews')
import holoviews as hv
import geoviews as gvI actually have a slight personal preference to do something like that; I think user experience is improved when common errors include debugging hints. But, I'm happy to go with whatever we decide together. |
|
Quick note: #1563 ran into issue with |
erogluorhan
left a comment
There was a problem hiding this comment.
Please resolve the merge conflicts so that I can see the test results before a larger review, but here are my initial thoughts about this work:
- Really appreciate the detailed description of this PR; it is extremely helpful!
- Per @rajeeja 's comment, I agree this is a major change and definitely requires being very careful; a few thoughts:
- conda does not have built-in optional dependencies like pip. Mechanisms such as lazy imports and fail locally rather than fail on installation will be needed in place to make sure there is no divergence-as much as possible- between conda and pip
- Per @Sevans711 's suggestion, clear guidance messages about the expected optional installs on both conda and pip will be needed moving forward.
- It is possible for some packages, e.g. datashader, that importing them will still be required even if their functionality is not explicitly called in the code (due to how the upstream, e.g. holoviz, dynamically uses them, for instance rasterization methods)
- Removing such packages might not be viable, so extensive testing of these changes will be needed before a possible merge
- Maybe most importantly, due to the all mentioned above, I'd recommend to take some time on this, and maybe look into similar but smaller dependency-related tasks first.
|
@erogluorhan thank you for looking into this! Replying below with what I hope is a concrete action plan (see bottom; apologies this message ended up being very long!). Includes a few questions for your consideration, in bold; I will wait for your thoughts on those before I proceed. We can of course iterate more after it is implemented, but for now I hope to clarify the immediate next steps. Minimize divergence between conda and pip:
I believe these mechanisms are already in place in this PR, but the relevant tests are not. I would propose: add these not-all-optional-dependencies pip builds to the autotests:
(we don't need to add pip install "uxarray[complete]", because that's what the current autotest pipeline does already.) And, run four test cases in each:
Would that be a sufficient set of tests, or should we aim for more than that? Clear guidance messages about optional installs
I am reading this as confirmation that you would maybe appreciate something like my suggested Beyond that, the PR currently includes updates to the uxarray readthedocs installation page, to help clarify how to do install with none/some/all optional dependencies. Silent functionality changes if packages missing
Thank you for raising these concerns. Considering more closely all the packages being removed:
I think this will reduce the amount of testing needed, as the "surface area" of possible effects of removing packages is decreased with these clarifications and change (moving datashader to viz instead of deleting it entirely). Taking time to make sure it works
Taking some time and being careful definitely makes sense.
EDIT: ah, I just saw your comment on #1575. That is a good example. Another option is to open another PR like "move should-be-optional dependency imports inside functions instead of at top of files" which could include all the changes to .py files here, but none of the changes to pyproject.toml and installation.rst. That would duplicate some effort, but it would allow the PR here to instead touch only the pyproject.toml, installation.rst, and yet-to-be-added optional-dependency-installation test files, which seem to be more contentious / needs caution than just relocating some imports. Do you think the duplicated effort is worth the ability to reduce the size of this PR? Action plan / summarySummary of changes discussed above / action plan:
|
Closes #1224
Waiting for #1541 before asking to merge (in order to move hvplot from required to "viz").Overview
Addresses user feedback asking to handle dependencies in a more elegant manner. Many previously-required dependencies are now optional. Also may speed up "import uxarray" slightly, as now-optional dependencies are moved to no longer be at top-level / top of files; instead, they are imported the first time any relevant method gets called. Imports of some required dependencies were similarly moved, to possibly improve "import uxarray" timing.
Notes about all dependencies which were previously listed as required in pyproject.toml:
antimeridian: important for geometry → keep as required. [EFF] Efficiency note: antimeridian was already never being imported at top-level.cartopy: used only in plotting → vizdask[dataframe]: keep as required for now. But, it is only used in core.aggrecation and core.zonal, so maybe could make optional? [EFF] already never at top-level.datashader: not used explicitly anywhere → removedgeoviews: used only in examples, for plotting → vizholoviews: used only in examples, tests, and HoloviewsBackend for plotting → vizmatplotlib: used only for plotting → vizmatplotlib-inline: not used explicitly anywhere → removednetcdf4: not used explicitly, but everyone is reading .nc files → keep as requirednumba: keep as required, of coursenumpy: keep as required, of coursepandas: important for grid methods, e.g. pd.Series and pd.IntervalIndex used explicitly → keep as requiredpyarrow: not used explicitly, and dask[dataframe] already depends on it → removedrequests: not used explicitly anywhere in uxarray → removedscikit-learn: important for neighbor algorithms, e.g. KDTree → keep as required. [EFF] already not at top-level.scipy: important for delaunay triangulations and some io → keep as required. [EFF] removed from top-level.shapely: important for geometry → keep as required. [EFF] already not at top-level.spatialpandas: just for spatialpandas.GeoDataFrame conversions; should be optional → geogeopandas: just for geopandas.GeoDataFrame conversions; should be optional → geoxarray: keep as required, of coursehvplot: used only for plotting → viz.Didn't move it yet though; waiting for Speed up import and load hvplot lazily #1541 to get mergedhealpix: just for healpix grids; should be optional → geopolars: used explicitly for some internal methods → keep as requiredpyproj: only used in uxarray.cross_sections.sample.sample_geodesic; should be optional → geoMiscellaneous notes:
pathlib; pathlib became a built-in part of the standard library in Python 3.4, and the pyproject.toml requires Python >= 3.10.poochto "dev" dependency group, because pooch was secretly required in order for all tests to pass. Comment in test/io/test_structure.py clarifies why pooch dependency exists despite not being imported explicitly anywhere in uxarray.Expected Usage
Users can now install uxarray with optional dependency groups. Simple examples with pip interface:
Required dependencies only:
Also includes all optional dependencies related to geospatial grids (geopandas, healpix, pyproj, spatialpandas)
Also includes everything related to geospatial grids, plus everything related to visualization (cartopy, geoviews, holoviews, matplotlib)
Includes all required plus all optional dependencies:
Equivalent to above:
pip install "uxarray[all]"PR Checklist
General
Testing
import uxarrayafter doingpip install uxarraywith minimal dependencies? We could maybe add a few more tests for different combinations of dependencies too?