Skip to content
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.

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

BLD: do not install *.pyx *.c MANIFEST.in #1200

Merged
merged 1 commit into from Nov 23, 2013

Conversation

josef-pkt
Copy link
Member

remove *.pyx *.c MANIFEST.in

this should fix and close #1195 cython pyximport tries to recompile the statsmodels extensions

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d34edff on josef-pkt:fix_manifest into 9d4b1f8 on statsmodels:master.

@jseabold
Copy link
Member

Don't we need also to adjust setup.py to ignore these? MANIFEST.in will take care of the source distributions only. Sorry the extent of the problem and the use of pyxinstall isn't clear to me.

@josef-pkt
Copy link
Member Author

AFAIR: roughly: setup.py controls what is included in the distribution archives, MANIFEST.in controls which extra files are installed

We still want to distribute the pyx and c files (we need the c files in the source distribution), but we don't want to install them, i.e. have them moved into the installed directories

so we need to keep them in the setup.py.

@josef-pkt
Copy link
Member Author

Forgot to mention in the description: I haven't tested this.

@jseabold
Copy link
Member

This works for the *.c files. They are not installed now, but will be included in the source distributions because they're not explicitly excluded. It does not work for the *.pyx files, though, so the problem still exists. They are still installed unless we explicitly exclude them, but then they will not be included in the source distributions.

We have a few options, but the easiest is if you just apply the below patch. As part of the ARIMA speed-up PR, I'll probably rearrange the Cython code everywhere to live in a src/ directory that we just don't install.

From d669978f5b728e78ad00e67ebb4c00541616932b Mon Sep 17 00:00:00 2001
From: Skipper Seabold <jsseabold@gmail.com>
Date: Sat, 23 Nov 2013 13:29:55 +0000
Subject: [PATCH] BLD: Do not install pyx files

---
 setup.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/setup.py b/setup.py
index a1411f1..ed08367 100644
--- a/setup.py
+++ b/setup.py
@@ -401,6 +401,7 @@ def pxd(name):
     return os.path.abspath(pjoin('pandas', name + '.pxd'))

 extensions = []
+exclude_package_data = {}
 for name, data in ext_data.items():
     sources = [srcpath(data['pyxfile'], suffix=suffix, subdir='')]
     pxds = [pxd(x) for x in data.get('pxdfiles', [])]
@@ -418,6 +419,12 @@ for name, data in ext_data.items():
                     include_dirs=include)

     extensions.append(obj)
+    # so that .pyx files are not installed
+    exclude_package_data.update(
+         # replace path separator with .
+        {re.sub('[\\\/]', '.',
+            pjoin('statsmodels', os.path.dirname(data['pyxfile']))) :
+            ["*.pyx"]})

 if suffix == '.pyx' and 'setuptools' in sys.modules:
     # undo dumb setuptools bug clobbering .pyx sources back to .c
@@ -509,4 +516,5 @@ if __name__ == "__main__":
           packages = packages,
           package_data = package_data,
           include_package_data=True,
+          exclude_package_data=exclude_package_data,
           **setuptools_kwargs)
-- 
1.8.3.2

@josef-pkt
Copy link
Member Author

can you try with adding '*.pyx' to the last line of MANIFEST.in?

global-exclude *~ *.swp *.pyc *.bak *.pyx

I don't think we need to change setup.py
From my reading of your patch, I think this won't include the pyx files in the source distribution anymore.

If I don't see this correctly, I need to go back to reading some documentation, and see which of my versions replicates this.

@jseabold
Copy link
Member

We don't need them in the sdist (it doesn't require Cython), but both keep them out of sdist apparently. I thought manifeset.in was only sdist and that this might avoid it. That's all that's mentioned in the docs for it.

http://docs.python.org/2/distutils/sourcedist.html#manifest

AFAIK, setup.py is the only way to exclude from install but not the sdist, though this patch doesn't do that.

@josef-pkt
Copy link
Member Author

As I set before: When I looked at this initially and created MANIFEST.in, the information I got was MANIFEST.in controls which extra files are installed

(original http://mail.scipy.org/pipermail/scipy-dev/2009-August/012741.html )

Interesting link (I was reading python documentation in python 2.4 or 2.5)
looks like python got a bit weird for a while

Changed in version 2.7: An existing generated MANIFEST will be regenerated without sdist comparing its modification time to the one of MANIFEST.in or setup.py.

Changed in version 2.7.1: MANIFEST files start with a comment indicating they are generated. Files without this comment are not overwritten or removed.

Changed in version 2.7.3: sdist will read a MANIFEST file if no MANIFEST.in exists, like it did before 2.7.

As far as I understood, our MANIFEST.in should never be overwritten or regenerated.

@jseabold
Copy link
Member

Ok...well add to exclude *pyx, and I think we can merge this.

@josef-pkt
Copy link
Member Author

If the change to manifest.in with global-exclude *.pyx works, then we could just change that for now and in 0.5.1.
It's a simpler change, then adjusting setup.py.
(Distributing the pyx files in the sdist wouldn't hurt and might be useful if someone uses a different cython version.)

I will do a round of testing with several python and dependency versions once 0.5.x has all the changes for 0.5.1

@jseabold
Copy link
Member

Changing MANIFEST.in will exclude them from the source distribution.

@josef-pkt
Copy link
Member Author

amended commit and force pushed

@jseabold
Copy link
Member

To be clear. Though I don't particularly care. If someone wants to muck around with different cython versions, they have to use the git source anyway. No Cython is used on installs from a source distribution.

@josef-pkt
Copy link
Member Author

you can push the green button if it looks right to you

@josef-pkt
Copy link
Member Author

To be clear. Though I don't particularly care. If someone wants to muck
around with different cython versions, they have to use the git source
anyway. No Cython is used on installs from a source distribution.

I agree, although this wasn't clear to me.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling ba5023e on josef-pkt:fix_manifest into 9d4b1f8 on statsmodels:master.

@josef-pkt
Copy link
Member Author

just a clarification to the python docs: there is the user defined MANIFEST.in and the autogenerated MANIFEST

AFAIU: we can include files in the sdist, but not install them, if we include them in the setup.py package_data but exclude them in MANIFEST.in

jseabold added a commit that referenced this pull request Nov 23, 2013
BLD: do not install *.pyx *.c  MANIFEST.in
@jseabold jseabold merged commit fb72fe4 into statsmodels:master Nov 23, 2013
jseabold added a commit that referenced this pull request Nov 23, 2013
remove *.pyx *.c  MANIFEST.in

this should fix and close #1195  cython pyximport tries to recompile the statsmodels extensions
@josef-pkt josef-pkt deleted the fix_manifest branch December 16, 2013 00:07
PierreBdR pushed a commit to PierreBdR/statsmodels that referenced this pull request Sep 2, 2014
BLD: do not install *.pyx *.c  MANIFEST.in
yarikoptic added a commit to yarikoptic/statsmodels that referenced this pull request Oct 23, 2014
* commit 'v0.5.0-13-g8e07d34':
  Backport PR statsmodels#1200: BLD: do not install *.pyx *.c  MANIFEST.in
  Backport PR statsmodels#1157: Tst precision master
  Backport PR statsmodels#1149: BUG: Fix small data issues for ARIMA.
  Backport PR statsmodels#1125: REF/BUG: Some GLM cleanup. Used trimmed results in NegativeBinomial variance.
  Backport PR statsmodels#1124: BUG: Fix ARIMA prediction when fit without a trend.
  Backport PR statsmodels#1117: Update ex_arma2.py
  Backport PR statsmodels#1089: ENH: exp(poisson.logpmf()) for poisson better behaved.
  Backport PR statsmodels#1077: BUG: Allow 1d exog in ARMAX forecasting.
  Backport PR statsmodels#1075: BLD: Fix build issue on some versions of easy_install.
  Backport PR statsmodels#1071: Update setup.py to fix broken install on OSX
  Backport PR statsmodels#1057: COMPAT: Fix py3 caching for get_rdatasets.
  BUG: fix predict (was refactoring victim)
yarikoptic added a commit to yarikoptic/statsmodels that referenced this pull request Oct 23, 2014
* origin/maintenance/0.5.x: (1875 commits)
  Backport PR statsmodels#1200: BLD: do not install *.pyx *.c  MANIFEST.in
  Backport PR statsmodels#1157: Tst precision master
  Backport PR statsmodels#1149: BUG: Fix small data issues for ARIMA.
  Backport PR statsmodels#1125: REF/BUG: Some GLM cleanup. Used trimmed results in NegativeBinomial variance.
  Backport PR statsmodels#1124: BUG: Fix ARIMA prediction when fit without a trend.
  Backport PR statsmodels#1117: Update ex_arma2.py
  Backport PR statsmodels#1089: ENH: exp(poisson.logpmf()) for poisson better behaved.
  Backport PR statsmodels#1077: BUG: Allow 1d exog in ARMAX forecasting.
  Backport PR statsmodels#1075: BLD: Fix build issue on some versions of easy_install.
  Backport PR statsmodels#1071: Update setup.py to fix broken install on OSX
  Backport PR statsmodels#1057: COMPAT: Fix py3 caching for get_rdatasets.
  BUG: fix predict (was refactoring victim)
  DOC: Update release notes with maint branch changes.
  MAINT: Fix mailmap entry.
  BUG: fix warning arguments in GenericLikelihoodModel
  MAINT: Add name to .mailmap.
  ENH: Pandas Series no longer inherits from ndarray. Closes statsmodels#1036.
  TST: Fixed test for Anaconda on Windows
  TST: Make test compatible with pandas 0.7.x
  BUG: Fail gracefully when not enough obs given for order.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pyximport.install() before import api crash
3 participants