Skip to content

Improve the way to import optional modules #2809

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
seisman merged 10 commits into from
Dec 4, 2023
Merged

Improve the way to import optional modules #2809

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5deebf8
Use a bool variable to indicate whether an optional module is available
seisman Nov 14, 2023
c98c103
Use upper-case for constant name (Fix C0103)
seisman Nov 14, 2023
b1c40b0
Use importlib.util.find_spec for modules that are not used (ruff F401)
seisman Nov 14, 2023
cdf162c
rioxarray needs to be imported
seisman Nov 14, 2023
0ec058f
Also ignore pylint warning
seisman Nov 14, 2023
3230ec4
Merge branch 'main' into optional-imports
seisman Nov 16, 2023
ef63776
Merge branch 'main' into optional-imports
seisman Nov 18, 2023
75ea9a4
Merge branch 'main' into optional-imports
seisman Nov 21, 2023
b37b18c
Update pygmt/src/tilemap.py
seisman Nov 21, 2023
7da73b9
Merge branch 'main' into optional-imports
seisman Dec 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions pygmt/datasets/tile_map.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@

try:
import contextily

_HAS_CONTEXTILY = True
except ImportError:
contextily = None
_HAS_CONTEXTILY = False

import numpy as np
import xarray as xr
Expand Down Expand Up @@ -108,7 +110,7 @@ def load_tile_map(region, zoom="auto", source=None, lonlat=True, wait=0, max_ret
* x (x) float64 -2.004e+07 -1.996e+07 ... 1.996e+07 2.004e+07
"""
# pylint: disable=too-many-locals
if contextily is None:
if not _HAS_CONTEXTILY:
raise ImportError(
"Package `contextily` is required to be installed to use this function. "
"Please use `python -m pip install contextily` or "
Expand Down
8 changes: 5 additions & 3 deletions pygmt/figure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@

try:
import IPython

_HAS_IPYTHON = True
except ImportError:
IPython = None # pylint: disable=invalid-name
_HAS_IPYTHON = False


from pygmt.clib import Session
Expand All @@ -33,7 +35,7 @@
}

# Show figures in Jupyter notebooks if available
if IPython:
if _HAS_IPYTHON:
get_ipython = IPython.get_ipython() # pylint: disable=invalid-name
if get_ipython and "IPKernelApp" in get_ipython.config: # Jupyter Notebook enabled
SHOW_CONFIG["method"] = "notebook"
Expand Down Expand Up @@ -452,7 +454,7 @@ def show(self, dpi=300, width=500, method=None, waiting=0.5, **kwargs):
)

if method == "notebook":
if IPython is None:
if not _HAS_IPYTHON:
raise GMTError(
"Notebook display is selected, but IPython is not available. "
"Make sure you have IPython installed, "
Expand Down
8 changes: 5 additions & 3 deletions pygmt/src/tilemap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@
from pygmt.helpers import build_arg_string, fmt_docstring, kwargs_to_strings, use_alias

try:
import rioxarray
import rioxarray # noqa: F401 # pylint: disable=unused-import
Comment thread
seisman marked this conversation as resolved.
Outdated

_HAS_RIOXARRAY = True
except ImportError:
rioxarray = None
_HAS_RIOXARRAY = False


@fmt_docstring
Expand Down Expand Up @@ -114,7 +116,7 @@ def tilemap(
"""
kwargs = self._preprocess(**kwargs) # pylint: disable=protected-access

if rioxarray is None:
if not _HAS_RIOXARRAY:
raise ImportError(
"Package `rioxarray` is required to be installed to use this function. "
"Please use `python -m pip install rioxarray` or "
Expand Down
13 changes: 5 additions & 8 deletions pygmt/tests/test_figure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,19 @@

Doesn't include the plotting commands which have their own test files.
"""
import importlib
import os
from pathlib import Path

try:
import IPython
except ImportError:
IPython = None # pylint: disable=invalid-name

@weiji14 weiji14 Nov 14, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option looking at https://stackoverflow.com/questions/61384752/how-to-type-hint-with-an-optional-import/77341843#77341843 seems to be something like:

IPython: Any = None
try:
    import IPython
except ImportError:
    pass

Though not sure if this is recommended.

@seisman seisman Nov 14, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unclear what the recommended way is for importing an optional module. It seems sometimes pandas and xarray use the has_xxx bool variable:

https://github.com/pandas-dev/pandas/blob/feca574525b2c37fac8d1d26bf82d428b95630c8/pandas/compat/compressors.py#L11

https://github.com/pandas-dev/pandas/blob/feca574525b2c37fac8d1d26bf82d428b95630c8/pandas/io/formats/style.py#L84

https://github.com/pydata/xarray/blob/49bd63a8332c1930a866724a2968b2d880dae25e/xarray/core/ops.py#L15

but sometimes they also use the old way, which mypy should complain, e.g.,
https://github.com/pydata/xarray/blob/49bd63a8332c1930a866724a2968b2d880dae25e/xarray/coding/times.py#L31

@weiji14 weiji14 Nov 14, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at mypy (e.g. python/mypy#1297 (comment)), they also seem to recommend a try-except-else pattern? Something like:

try:
    import IPython as _IPython
except ImportError:
    IPython = None
else:
    IPython = _IPython

@seisman seisman Nov 14, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's officially recommended and it's also more difficult to understand than IPython = None and _HAS_IPYTHON = False.

@weiji14 weiji14 Nov 16, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's go with _HAS_IPython unless there's a better recommended way 🙂

That said, we should probably decide first if adding mypy is something we want (#2808), otherwise the changes here aren't really needed.

@seisman seisman Nov 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're going to add type hints (#2794, #2812), we definitely need a static type checker. There are many static type checkers, like mypy, pyright, and pytype, in which mypy is the official one, so it's a good option to start.



import numpy as np
import numpy.testing as npt
import pytest
from pygmt import Figure, set_display
from pygmt.exceptions import GMTError, GMTInvalidInput
from pygmt.helpers import GMTTempFile

HAS_IPYTHON = bool(importlib.util.find_spec("IPython"))


def test_figure_region():
"""
Expand Down Expand Up @@ -313,7 +310,7 @@ def test_figure_savefig_worldfile():
fig.savefig(fname=imgfile.name, worldfile=True)


@pytest.mark.skipif(IPython is None, reason="run when IPython is installed")
@pytest.mark.skipif(not HAS_IPYTHON, reason="run when IPython is installed")
def test_figure_show():
"""
Test that show creates the correct file name and deletes the temp dir.
Expand Down Expand Up @@ -354,7 +351,7 @@ def test_figure_show_invalid_method():
fig.show(method="test")


@pytest.mark.skipif(IPython is not None, reason="run without IPython installed")
@pytest.mark.skipif(HAS_IPYTHON, reason="run without IPython installed")
def test_figure_show_notebook_error_without_ipython():
"""
Test to check if an error is raised when display method is 'notebook', but
Expand Down