BLD lazy import of tempita to avoid cython dependency#15313
BLD lazy import of tempita to avoid cython dependency#15313rth merged 7 commits intoscikit-learn:masterfrom
Conversation
sklearn/utils/setup.py
Outdated
|
|
||
| with open(pyxfiles, "r") as f: | ||
| tmpl = f.read() | ||
| from Cython import Tempita # noqa |
There was a problem hiding this comment.
Could this be a function in sklearn/_build_utils/__init__.py. Something along these lines:
def tempita_sub(*args, **kwargs):
"""Helper function.
This does lazy import of Cython to make sure that we do not depend on Cython when building from a release sdist (since it contains the .c files already)
"""
from Cython import Tempita
Tempita.sub(*args, **kwargs)There was a problem hiding this comment.
Or adding a comment above the import about it.
rth
left a comment
There was a problem hiding this comment.
LGTM, aside for the comment below.
sklearn/utils/setup.py
Outdated
|
|
||
| with open(pyxfiles, "r") as f: | ||
| tmpl = f.read() | ||
| from Cython import Tempita # noqa |
There was a problem hiding this comment.
Or adding a comment above the import about it.
|
I went for the utility fonction |
|
|
||
| import numpy | ||
|
|
||
| from sklearn._build_utils import gen_from_templates |
There was a problem hiding this comment.
out of curiosity, can you explain me why an absolute import is required here ?
There was a problem hiding this comment.
It doesn't work if we use a relative import?
There was a problem hiding this comment.
No, they also did it for the deprecated modules in sklearn/setup.py
Maybe @thomasjpfan knows better about that ?
NicolasHug
left a comment
There was a problem hiding this comment.
A few comments but looks good
NicolasHug
left a comment
There was a problem hiding this comment.
Thanks @jeremiedbb , I think it's clear now.
LGTM, though I'm still curious about the need for the absolute import
Fixes #15298
Same fix as #13980.
There are no other occurrence.