setup.py that works with pip and doesn't rely on Cython for source distributions #39
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…j-like; correctly referred to Eigen in deps everywhere; finalized the clean mode for developers
This was referenced Feb 28, 2015
|
Since cythonize checks timestamps, why is having a (If it is a good idea, it may be better to remove only .c/.cpp files that correspond to .pyx files instead of assuming all .c/.cpp files should be removed. A good way to do that would be to map a 'clean' function over the extension list that is already created by globbing for .pyx files. But I'd like to understand why it's necessary first.) |
|
Can you explain what you mean by "(ii) correctly distributes Cython-generated files when making a source distribution"? What does the current setup.py do incorrectly? |
|
The |
|
In those three features (including "(ii) correctly distributes Cython-generated files when making a source distribution") I just meant to describe the features the |
|
Please hold off on this pull request. Need more extensive testing... |
…iles themselves. There are too many places where the Eigen dir is mentioned - it is in setup.py, in __init__.py, and in individual *.pyx files...
…tter testing suite.
…included in git through .ctags
…to be used in setup.py
|
OK, finally ready. I made some changes and it'd be great to get your input.
This line will just run any file anywhere in the package that's named Here's a complete log of how I tested this version of the code, first in developer-mode and then as a user of stable release. Testing as a developerStarting in the Clean files (optional): now install as developer: Since I use virtual environments with pip, I add this to my virtual environment: Now run the unit tests: I consider this to mean it's working. We should add more unit tests for better coverage of package, but for now I'm using this as the read out. Now to test if it works as source distribution like the one that would be downloadable from pypi. Testing as a user of source distribution/stable releaseClean all previous build/Cython-generated code: Then create a source distribution: Then transfer the source distribution ( The source distribution looks like this: Now install it with pip. This should not depend on Cython, but simply compile the Cython-generated C++: As the output above says, it's not using Cython, just compiling the Cython-generated code. Now run the unit tests: So it looks like it's working. There's one part that I cannot figure out for the life of me, and after beating my head against it for two days I have given up. If in the above installation procedure I do |
|
Hi Matt, just wondering if you'd had a chance to look at it? Thanks, Yarden |
|
I've finally got a chance to take a look at this! I made a merge-able version on a local branch. Here's my attempted re-outline of what this PR does:
Can you explain 4? I'm not sure what the setup.py in the current master is lacking; I use it both to build from cython sources and to create source distributions, and installing via pypi seems to work. Can you spell out the use case that isn't covered in master but is patched up by this PR? I'm going to look into some tweaks to 2 and 3, namely the clean command should probably follow the distutils custom command style and the tests should probably just be nose-runnable. |
|
I wrote a new setup.py that should address most of these concerns (except it still has crappy default cleaning). I'm going to reread this thread to see what I missed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A revised setup.py that: (i) works with
pip install, (ii) correctly distributes Cython-generated files when making a source distribution, and (iii) doesn't rely on Cython being installed when installing a source distribution (just compiles the Cython-generated cpp code).A couple of other changes in this pull request:
deps/outside ofpyhsmm/pyhsmm/cleanoption tosetup.pythat removes Cython-generated code (*.cpp, *.c, *.so) and build-related files. Intended to be used only by developers - very useful for testing code. Note that this relies on all the .cpp/.c files inpyhsmm/being Cython-generated.developer_notes.md- if these are useful they can be incorporated into the README.md or to a separate developer's pyhsmm guide if you make one.Here's how I tested the source distribution creation/installation:
To test the installation from the github repo, I just did:
This will apply Cython to the *.pyx files to generate the cpp code, and then install the package.
If both of these work, then we're good for both the automatic installation with pip and the developer's build case.
If that sounds good you, I can make a similar fix for
pybasicbayes(to make it handle Cython correctly), and then add travis CI support forpyhsmmto make the testing more streamlined. The only missing ingredient then would be a unit testing main driver script.