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

No cython to install #20

Merged
merged 14 commits into from Jun 10, 2019

Conversation

Projects
None yet
2 participants
@sixpearls
Copy link
Collaborator

commented Jun 9, 2019

This is a start for #18 -- I believe it removes Cython as a requirement to install. I just checked, and it seems like NumPy does NOT need to be pre-installed to compile.

I am working on including the NumPy implementation in #19, but I am not sure how to test setup without the compiler. I tried installing without NumPy or Cython pre-installed. After pip installed the NumPy requirement, it seemed to go back to compile the .c file.

@ixjlyons
Copy link
Member

left a comment

This looks good to me.

As long as the C file is included in the sdist package, it should work, right? I believe the C generated by cython is portable, and it looks like the C code is already included in the sdist: https://docs.python.org/3.7/distutils/sourcedist.html#specifying-the-files-to-distribute

After generating _bspl.c with cython, I started over in a new venv, then pip install . brought in numpy and scipy (it does pull the wheels, which I was concerned about actually) and built the .so file, which I think simulates the user experience accurately if installing from the sdist on PyPI with nothing installed (except C compiler).

Maybe go ahead with this PR and work on the numpy-only related packaging over in the other PR or a separate one?

setup.py Outdated

if use_cython and use_numpy:
extensions = cythonize([
Extension(f"{name}._bspl",

This comment has been minimized.

Copy link
@ixjlyons

ixjlyons Jun 9, 2019

Member

Should we make an extension = "_bspl" variable and if you're cool with requiring Python >= 3.6, use f strings like here to replace all occurrences of _bspl with {extension}.

This comment has been minimized.

Copy link
@ixjlyons

ixjlyons Jun 9, 2019

Member

A lot of projects are moving on to require > 3.4, but I think 3.5 support is still pretty widespread. For me, f strings are nice but not really worth being the sole reason for a Python version requirement.

This comment has been minimized.

Copy link
@sixpearls

sixpearls Jun 9, 2019

Author Collaborator

I neither feel motivated to support <= 3.5 nor do I feel strongly about f-strings over some other way to DRY up references to the name. I guess we should be consistent though? If you don't mind I'll defer to you on this. I believe the f-string usage here came from your commit.

setup.py Outdated
]
class sdist(_sdist):
def run(self):
cythonize([os.path.join(name, "{}.pyx".extension())])

This comment has been minimized.

Copy link
@ixjlyons

ixjlyons Jun 9, 2019

Member

Ok I just used format strings. I also changed this (used to be cythonize(['cython/mycythonmodule.pyx'])) which I think just got left over from copying the template.

This comment has been minimized.

Copy link
@ixjlyons

ixjlyons Jun 9, 2019

Member

Also just realized I don't like using the name extension and I screwed this one up. Should be fixed now

@sixpearls

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 9, 2019

Turns out we don't need to override the sdist command at all (anymore?).

In my environment (python 3.6) with Cython installed, I can run python setup.py sdist, and it will cythonize the extension and generate the .c file (if it doesn't exist or (I assume) if it is stale) and add it to the egg-info\SOURCES.txt file. This makes me think doing an sdist upload to release a version to PyPI will work.

So with #19, this should be pip installable now?

Should we add the .c file to the repository so that you could install from the repository without Cython? Or say that if you are installing from the repository, you have to use Cython? Leaning towards the latter.

@ixjlyons

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

👍 for leaving _bspl.c out of git

Can always upload a dry run to https://test.pypi.org/ to check that everything works.

@sixpearls

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 9, 2019

I'm not sure why, but when I try to install from PyPI my compiler complains about not finding the header file. Could you try? Use

$ python -m pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple ndsplines

to pull ndsplines from test and numpy/scipy from production

@ixjlyons

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

Looks like we just need to include ndsplines/_bspl.h in the sdist package in MANIFEST.in see here

@sixpearls

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 10, 2019

I was just looking at that. It looks like I can add the .h file to the second argument and it seems to work. I am not sure why I can't find a docs for the Extension class.

Do you mind trying again?

Uploading, after installing twine, is:

.. code:: bash
$ python setup.py sdist
$ python -m twine upload --repository-url https://test.pypi.org/legacy/ dist/*

@ixjlyons

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

Seems to be working for me.

@sixpearls sixpearls merged commit 4391d3d into master Jun 10, 2019

@sixpearls

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 10, 2019

Okay, this worked on a computer with no compiler (although I suppose I'm not sure that the optional flag was necessary, I think it should have been).

Merging and closing everything.

@sixpearls sixpearls deleted the no_cython_to_install branch Jun 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.