Skip to content

Loading…

[WIP] Do not import the sklearn package from setup.py #3015

Closed
wants to merge 2 commits into from

6 participants

@ogrisel
scikit-learn member

The execution of setup.py never imports the sklearn package to avoid cyclid dependencies.

The sklearn/version.py file is now generated each time setup.py is executed. If the version is not a release version, a '.dev_aaaaaaa' suffix is appended where 'aaaaaaa' is the 7 first digits of the hexdigest of the last git commit in the local repo.

I find this solution less hackish and more informative than the previous stateful builtins-based hack.

@ogrisel ogrisel Do not import sklearn package from setup.py and generate better dev v…
…ersion

The execution of setup.py never imports the sklearn package to avoid cyclid
dependencies.

The sklearn/version.py file is now generated each time setup.py is executed.
If the version is not a release version, a '.dev_aaaaaaa' suffix is appended
where 'aaaaaaa' is the 7 first digits of the hexdigest of the last git
commit in the local repo.
6136990
@ogrisel ogrisel added the Enhancement label
@ogrisel ogrisel added this to the 0.15 milestone
@ogrisel ogrisel changed the title from [MRG] Do not import the sklearn package from setup.py to [WIP] Do not import the sklearn package from setup.py
@ogrisel
scikit-learn member

Oops, as travis revealed, the builtins hack is not just necessary for the version stuff but also for package introspection by numpy.distutils that does trigger the sklearn package import... I will try to come up with a better solution.

@ogrisel ogrisel removed this from the 0.15 milestone
@fareshedayati
@ogrisel ogrisel changed the title from [WIP] Do not import the sklearn package from setup.py to [MRG] Do not import the sklearn package from setup.py
@ogrisel
scikit-learn member

OK I restored the buitins protection. I am wondering if we can do better though.

@jakevdp
scikit-learn member

This is a really nice solution! You should add it to the reddit thread :smile:

@ogrisel ogrisel changed the title from [MRG] Do not import the sklearn package from setup.py to [WIP] Do not import the sklearn package from setup.py
@jnothman jnothman commented on the diff
@@ -20,20 +38,71 @@
# avoid attempting to load components that aren't built yet.
builtins.__SKLEARN_SETUP__ = True
-DISTNAME = 'scikit-learn'
-DESCRIPTION = 'A set of python modules for machine learning and data mining'
-LONG_DESCRIPTION = open('README.rst').read()
-MAINTAINER = 'Andreas Mueller'
-MAINTAINER_EMAIL = 'amueller@ais.uni-bonn.de'
-URL = 'http://scikit-learn.org'
-LICENSE = 'new BSD'
-DOWNLOAD_URL = 'http://sourceforge.net/projects/scikit-learn/files/'
+# Return the git revision as a string
+def git_version():
+ def _minimal_ext_cmd(cmd):
+ # construct minimal environment
@jnothman scikit-learn member

Why is it necessary to synthesize the environment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jnothman jnothman commented on the diff
((10 lines not shown))
-LICENSE = 'new BSD'
-DOWNLOAD_URL = 'http://sourceforge.net/projects/scikit-learn/files/'
+# Return the git revision as a string
+def git_version():
+ def _minimal_ext_cmd(cmd):
+ # construct minimal environment
+ env = {}
+ for k in ['SYSTEMROOT', 'PATH']:
+ v = os.environ.get(k)
+ if v is not None:
+ env[k] = v
+ # LANGUAGE is used on win32
+ env['LANGUAGE'] = 'C'
+ env['LANG'] = 'C'
+ env['LC_ALL'] = 'C'
+ out = subprocess.Popen(cmd, stdout = subprocess.PIPE,
@jnothman scikit-learn member

you can just use subprocess.check_output(cmd, env=env)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jnothman jnothman commented on the diff
((17 lines not shown))
+ for k in ['SYSTEMROOT', 'PATH']:
+ v = os.environ.get(k)
+ if v is not None:
+ env[k] = v
+ # LANGUAGE is used on win32
+ env['LANGUAGE'] = 'C'
+ env['LANG'] = 'C'
+ env['LC_ALL'] = 'C'
+ out = subprocess.Popen(cmd, stdout = subprocess.PIPE,
+ env=env).communicate()[0]
+ return out
+
+ try:
+ out = _minimal_ext_cmd(['git', 'rev-parse', 'HEAD'])
+ GIT_REVISION = out.strip().decode('ascii')
+ except OSError:
@jnothman scikit-learn member

Is it possible for subprocess.CalledProcessError or IOError to fire?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jnothman jnothman closed this
@jnothman jnothman reopened this
@jnothman
scikit-learn member

Sorry about the close... This looks fine except for those small issues.

@coveralls

Coverage Status

Coverage remained the same when pulling ddd9113 on ogrisel:setup-version into 033ae09 on scikit-learn:master.

@larsmans larsmans commented on the diff
((36 lines not shown))
+
+
+def get_version_info():
+ FULLVERSION = VERSION
+ if os.path.exists('.git'):
+ GIT_REVISION = git_version()
+ else:
+ GIT_REVISION = "Unknown"
+
+ if not ISRELEASED:
+ FULLVERSION += '.dev-' + GIT_REVISION[:7]
+
+ return FULLVERSION, GIT_REVISION
+
+
+def write_version_py(filename='sklearn/version.py'):
@larsmans scikit-learn member

Please name it _version.py.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ogrisel
scikit-learn member

I will address your comments. I am also working a way to get rid of the sklearn imports in the setup.py files.

@ogrisel
scikit-learn member

Closing this PR:

  • the git based versioning scheme does not follow PEP 440 conventions,
  • the numpy-based build system mandates the import of the sklearn package anyway.

I will open a new PR with a PEP 440 compliant version scheme (for the dev release flag).

@ogrisel ogrisel closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 29, 2014
  1. @ogrisel

    Do not import sklearn package from setup.py and generate better dev v…

    ogrisel committed
    …ersion
    
    The execution of setup.py never imports the sklearn package to avoid cyclid
    dependencies.
    
    The sklearn/version.py file is now generated each time setup.py is executed.
    If the version is not a release version, a '.dev_aaaaaaa' suffix is appended
    where 'aaaaaaa' is the 7 first digits of the hexdigest of the last git
    commit in the local repo.
  2. @ogrisel
Showing with 94 additions and 25 deletions.
  1. +3 −0 .gitignore
  2. +83 −14 setup.py
  3. +8 −11 sklearn/__init__.py
View
3 .gitignore
@@ -52,3 +52,6 @@ benchmarks/bench_covertype_data/
*.prefs
.pydevproject
.idea
+
+# Generated when running setup.py
+sklearn/version.py
View
97 setup.py
@@ -8,8 +8,26 @@
import sys
import os
import shutil
+import subprocess
from distutils.command.clean import clean as Clean
+
+DISTNAME = 'scikit-learn'
+DESCRIPTION = 'A set of python modules for machine learning and data mining'
+LONG_DESCRIPTION = open('README.rst').read()
+MAINTAINER = 'Andreas Mueller'
+MAINTAINER_EMAIL = 'amueller@ais.uni-bonn.de'
+URL = 'http://scikit-learn.org'
+LICENSE = 'new BSD'
+DOWNLOAD_URL = 'http://sourceforge.net/projects/scikit-learn/files/'
+
+MAJOR = 0
+MINOR = 15
+MICRO = 0
+ISRELEASED = False
+VERSION = '%d.%d.%d' % (MAJOR, MINOR, MICRO)
+
+
if sys.version_info[0] < 3:
import __builtin__ as builtins
else:
@@ -20,20 +38,71 @@
# avoid attempting to load components that aren't built yet.
builtins.__SKLEARN_SETUP__ = True
-DISTNAME = 'scikit-learn'
-DESCRIPTION = 'A set of python modules for machine learning and data mining'
-LONG_DESCRIPTION = open('README.rst').read()
-MAINTAINER = 'Andreas Mueller'
-MAINTAINER_EMAIL = 'amueller@ais.uni-bonn.de'
-URL = 'http://scikit-learn.org'
-LICENSE = 'new BSD'
-DOWNLOAD_URL = 'http://sourceforge.net/projects/scikit-learn/files/'
+# Return the git revision as a string
+def git_version():
+ def _minimal_ext_cmd(cmd):
+ # construct minimal environment
@jnothman scikit-learn member

Why is it necessary to synthesize the environment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ env = {}
+ for k in ['SYSTEMROOT', 'PATH']:
+ v = os.environ.get(k)
+ if v is not None:
+ env[k] = v
+ # LANGUAGE is used on win32
+ env['LANGUAGE'] = 'C'
+ env['LANG'] = 'C'
+ env['LC_ALL'] = 'C'
+ out = subprocess.Popen(cmd, stdout = subprocess.PIPE,
@jnothman scikit-learn member

you can just use subprocess.check_output(cmd, env=env)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ env=env).communicate()[0]
+ return out
+
+ try:
+ out = _minimal_ext_cmd(['git', 'rev-parse', 'HEAD'])
+ GIT_REVISION = out.strip().decode('ascii')
+ except OSError:
@jnothman scikit-learn member

Is it possible for subprocess.CalledProcessError or IOError to fire?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ GIT_REVISION = "Unknown"
+
+ return GIT_REVISION
+
+
+def get_version_info():
+ FULLVERSION = VERSION
+ if os.path.exists('.git'):
+ GIT_REVISION = git_version()
+ else:
+ GIT_REVISION = "Unknown"
+
+ if not ISRELEASED:
+ FULLVERSION += '.dev-' + GIT_REVISION[:7]
+
+ return FULLVERSION, GIT_REVISION
+
+
+def write_version_py(filename='sklearn/version.py'):
@larsmans scikit-learn member

Please name it _version.py.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ content = """
+# THIS FILE IS GENERATED FROM SETUP.PY
+short_version = '%(version)s'
+version = '%(version)s'
+full_version = '%(full_version)s'
+git_revision = '%(git_revision)s'
+release = %(isrelease)s
+
+if not release:
+ version = full_version
+"""
+ FULLVERSION, GIT_REVISION = get_version_info()
+
+ a = open(filename, 'w')
+ try:
+ a.write(content % {'version': VERSION,
+ 'full_version' : FULLVERSION,
+ 'git_revision' : GIT_REVISION,
+ 'isrelease': str(ISRELEASED)})
+ finally:
+ a.close()
+ return FULLVERSION
-# We can actually import a restricted version of sklearn that
-# does not need the compiled code
-import sklearn
+FULLVERSION = write_version_py()
-VERSION = sklearn.__version__
###############################################################################
# Optional setuptools features
@@ -101,7 +170,7 @@ def setup_package():
description=DESCRIPTION,
license=LICENSE,
url=URL,
- version=VERSION,
+ version=FULLVERSION,
download_url=DOWNLOAD_URL,
long_description=LONG_DESCRIPTION,
classifiers=['Intended Audience :: Science/Research',
@@ -138,7 +207,7 @@ def setup_package():
except ImportError:
from distutils.core import setup
- metadata['version'] = VERSION
+ metadata['version'] = FULLVERSION
else:
from numpy.distutils.core import setup
View
19 sklearn/__init__.py
@@ -15,27 +15,24 @@
import sys
import re
import warnings
-__version__ = '0.15-git'
# Make sure that DeprecationWarning within this package always gets printed
warnings.filterwarnings('always', category=DeprecationWarning,
module='^{0}\.'.format(re.escape(__name__)))
try:
- # This variable is injected in the __builtins__ by the build
- # process. It used to enable importing subpackages of sklearn when
- # the binaries are not built
- __SKLEARN_SETUP__
+ # This builtins variable is defined by setup.py when building the project.
+ in_setup = __SKLEARN_SETUP__
except NameError:
- __SKLEARN_SETUP__ = False
+ in_setup = False
-if __SKLEARN_SETUP__:
- sys.stderr.write('Partial import of sklearn during the build process.\n')
- # We are not importing the rest of the scikit during the build
- # process, as it may not be compiled yet
-else:
+if not in_setup:
+ # Trigger the build check only when run once the build is actually
+ # complete.
from . import __check_build
from .base import clone
+ from .version import full_version as __version__
+
def test(*args, **kwargs):
import warnings
Something went wrong with that request. Please try again.