Revised setup.py and minor fixes in kernels. #89

JSKenyon · 2017-11-07T07:24:08Z

This should be a more general approach to setup which removes the Cython dependency. Assigning @gijzelaerr to check if this is more acceptable from a packaging perspective. Behaviour is modelled on rasterio, one of the more successful repositories I could find which also uses Cython. Default behaviour is to use .c and .cpp files instead of Cythonising. Will Cythonise locally if Cython is installed. This should be "safe" behaviour.

JSKenyon · 2017-11-23T08:35:03Z

Ok, @o-smirnov. I think this is ready to be merged.

o-smirnov · 2017-11-23T08:48:29Z

Has @gijzelaerr done a review?

JSKenyon · 2017-11-23T08:57:23Z

Not as far as I know. I have tested it to the best of my ability, so I think it works. The docs will become consistent with the new approach when it gets merged. It would be cool if @gijzelaerr could weigh in and let me know if the changes will make packaging it easier.

gijzelaerr · 2017-11-23T09:14:37Z

setup.py

+    import numpy as np
+except ImportError:
+    log.critical("Numpy and its headers are required to run setup(). Exiting.")
+    sys.exit(1)


don't raise exceptions or exit manually in the setup script. It should still be possible to run the setup.py script without any of the dependencies installed.

I am not sure I understand. Surely installation will fail if you have unmet dependencies?

well how is somebody going to be able to say what the dependencies are if the setup.py script fails to parse if the dependencies are not installed?

gijzelaerr · 2017-11-23T09:14:55Z

setup.py

+except ImportError:
+    cythonize = None
+
+include_path = np.get_include()


probably best to just make this an empty array if numpu is not installed

The whole installation will bomb without numpy - I don't think the .c and .cpp files can compile without those numpy headers.

opening, reading, parsing the setup.py file doesn't mean you are also actually installing the project.

gijzelaerr · 2017-11-23T09:17:03Z

setup.py

        "Programming Language :: Python",
-        "Topic :: Software Development :: Libraries :: Python Modules",
        "Topic :: Scientific/Engineering :: Astronomy"],
      author='Jonathan Kenyon',
      author_email='[email protected]',
      license='GNU GPL v3',
      cmdclass={'install': custom_install},  


I think this is very scary, a custom install command class that calls a python executable to add some arguments to the build_ext step. There must be a better way to do this.

I am open to suggestions. We are very much at the edge of my familiarity.

@gijzelaerr This is the kind of thing that anyone with a python package that builds an extension will run into at some point. its a chicken and egg scenario -- you need numpy/tensorflow/something to build your extension, but its not installed yet, so it needs to be put in install_requires so that numpy/tensorflow/something gets installed. But the setup.py script tries to build the extension first.

Hence:

PEP 518.

Install build requirements before calling setup.py pypa/pip#4799

Toward PEP 518 pypa/pip#4802

Until the above is fixed in some satisfactory way, deferred extension builds via sub-classing build_ext is the way to solve the chicken and egg scenario. See the datastax python driver setup.py, for example.

what are you trying to accomplish? What happens if you remove the custom install cmdclass?

In fact, it looks like PEP 518 has made it into pip pypa/pip#4144. A cursory read suggests one needs a pyproject.toml to specify build dependencies and break the chicken and egg links.

but that probably doesnt help @JSKenyon very much at the moment.

The changes I suggest is:

Don't exit on the failing numpy import

If you can't populate the list for now, create an empty list. Specify numpy as a build dependency in setup.py.

Don't call an external python interpreter, that looks like a a recipe for problems. Can't that flags be set somewhere internal?

JSKenyon · 2017-11-23T10:21:21Z

Working on this right now. Currently, it may be sufficient to just import build_ext. My net just died so will take me a little longer.

…

Sent from my iPhone

On 23 Nov 2017, at 11:50, Gijs Molenaar ***@***.***> wrote: @gijzelaerr commented on this pull request. In setup.py: > "Programming Language :: Python", - "Topic :: Software Development :: Libraries :: Python Modules", "Topic :: Scientific/Engineering :: Astronomy"], author='Jonathan Kenyon', ***@***.***', license='GNU GPL v3', cmdclass={'install': custom_install}, what are you trying to accomplish? What happens if you remove the custom install cmdclass? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

JSKenyon · 2017-11-23T12:21:26Z

@gijzelaerr I have made some changes that seem to work. Would you mind reviewing setup.py again? Not sure what the review procedure is. Do I dismiss the previous one?

JSKenyon · 2017-11-23T12:56:48Z

Thanks. Didn't know that was valid syntax.

gijzelaerr · 2017-11-23T13:02:47Z

it looks like the SharedArray dependency has the same issue:

Collecting sharedarray (from cubical==0.9.2)
  Downloading SharedArray-2.0.3.tar.gz
    Complete output from command python setup.py egg_info:
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/private/var/folders/qs/hpsrfjwx0313rdk2t7jj2ddw0000gn/T/pip-build-v99tMq/sharedarray/setup.py", line 23, in <module>
        import numpy
    ImportError: No module named numpy

Has this been reported upstream?

JSKenyon · 2017-11-23T13:14:25Z

@gijzelaerr It has not, but I feel you are in a better position to explain precisely what is wrong. Thus far, I have only worked around the dependencies. @o-smirnov you know the provenance of SharedArray more than I do. Any ideas?

sjperkins · 2017-11-23T13:17:11Z

SharedArray is running into the exact same problem that you are now :-)

sjperkins · 2017-11-23T13:17:45Z

https://github.com/ddboline/shared-array/blob/master/setup.py#L22

gijzelaerr · 2017-11-23T13:30:24Z

ok tried this on my mac but getting stuck at compiling casacore. I'll only be able to continue this on sunday.

JSKenyon · 2017-11-23T13:39:08Z

@gijzelaerr No stress - you are on holiday. I was expecting this to be ignored for a while. Enjoy Bahrain!

gijzelaerr · 2017-11-23T13:45:26Z

i'm actually working from here, but the middle-eastern weekend just started :)

o-smirnov · 2017-11-23T14:06:13Z

Issue 1 reported! ddboline/shared-array#1

sjperkins · 2017-11-23T16:27:20Z

@JSKenyon Try the sjp-rev-setup branch. The taste of dark chocolate is pleasing to me.

JSKenyon · 2017-11-24T07:53:55Z

@sjperkins Seems good, though Cython was not a problem - it doesn't need to be installed. It will be used if it is, otherwise it will default to using the bundled .c and .cpp files. @gijzelaerr Feel free to have a look at @sjperkins approach when you get a moment (zero pressure) and see if it meets with your approval. I found this yesterday. If it seems ok, might be another way to break the np.get_include() problem.

sjperkins · 2017-11-24T09:27:52Z

For reference's sake

gijzelaerr · 2017-11-26T11:23:04Z

for me this installs in a clean virtualenv with and without --system-site-packages. I think it is good to go.

gijzelaerr · 2017-11-26T12:19:21Z

i've made a package of this branch and uploaded it to KERN-dev and KERN-3 as 0.9.2.1, it seems to work. If you make a new release I'll update the package to that release.

So your docker instructions now are:

FROM kernsuite/base:3
RUN docker-apt-install cubical

o-smirnov · 2018-04-21T10:14:19Z

Can we revisit the issue of whether the *.c files need to be checked into the repository? @gijzelaerr it was at your instigation, wasn't it? Since it's auto-generated code, it feels a bit weird checking them in. Also makes for massive git diffs, making it harder to get an overview of commits. Cannot we generate them when shipping the package?

gijzelaerr · 2018-04-23T09:03:57Z

The Cython peeps recommend bundling your tarball with the generated C code so you don't depend on Cython. if you think it is easier to only upload upon a release then i guess it is okay, but i'm quite sure people are going to forget.

o-smirnov · 2018-04-23T09:43:10Z

but i'm quite sure people are going to forget.

But that's easy to catch -- the package doesn't build, we're alerted, we fix the tarball. This only happens once in a while, when we release, right?

Whereas having the .c files checked in is just daily misery.

gijzelaerr · 2018-04-23T09:45:31Z

ok lets see what happens :)

JSKenyon added 8 commits October 17, 2017 12:18

Adding compiled kernels and revised setup.

ba9ae51

Changed setup default to non-cython.

5c00d1e

Trying to make revised installer more robust.

4cdf1e1

More wok on setup.py.

301a284

Merge branch 'master' into rev-setup

3063f70

Crap work on crap branch.

b6879a6

Merge branch 'master' into rev-setup

b1dd1b0

Revised setup a little further. Added newer .c and .cpp files.

3466d6d

JSKenyon assigned gijzelaerr Nov 7, 2017

JSKenyon requested a review from gijzelaerr November 8, 2017 09:59

JSKenyon unassigned gijzelaerr Nov 8, 2017

JSKenyon added 6 commits November 22, 2017 11:28

Minor changes to setup script.

8de6504

Updating setup.py and the docs to be compatible with the new setup.

6dd195a

Fixing a typo in the installation instructions.

931e639

Fixing a typo in the installation instructions.

a1f5a10

Fixing a typo in the installation instructions.

e5e275b

Fixing versioning.

d2989a1

gijzelaerr requested changes Nov 23, 2017

View reviewed changes

JSKenyon mentioned this pull request Nov 23, 2017

python-casacore >= 2.1.2 to python-casacore == 2.1.2 #92

Closed

Minor fixes to setup.py.

f9a7ec3

Minor fix.

8f0387a

o-smirnov mentioned this pull request Nov 23, 2017

setup.py should not import numpy directly ddboline/shared-array#1

Closed

gijzelaerr approved these changes Nov 26, 2017

View reviewed changes

o-smirnov merged commit 3b39dd1 into master Nov 27, 2017

JSKenyon mentioned this pull request Dec 4, 2017

don't enable cython by default #71

Closed

JSKenyon deleted the rev-setup branch December 4, 2017 11:30

Revised setup.py and minor fixes in kernels. #89

Revised setup.py and minor fixes in kernels. #89

Conversation

JSKenyon commented Nov 7, 2017 • edited Loading

JSKenyon commented Nov 23, 2017

o-smirnov commented Nov 23, 2017

JSKenyon commented Nov 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gijzelaerr Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gijzelaerr Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjperkins Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JSKenyon commented Nov 23, 2017 via email

JSKenyon commented Nov 23, 2017 • edited Loading

JSKenyon commented Nov 23, 2017

gijzelaerr commented Nov 23, 2017

JSKenyon commented Nov 23, 2017 • edited Loading

sjperkins commented Nov 23, 2017

sjperkins commented Nov 23, 2017

gijzelaerr commented Nov 23, 2017

JSKenyon commented Nov 23, 2017

gijzelaerr commented Nov 23, 2017 • edited Loading

o-smirnov commented Nov 23, 2017

sjperkins commented Nov 23, 2017

JSKenyon commented Nov 24, 2017

sjperkins commented Nov 24, 2017 • edited Loading

gijzelaerr commented Nov 26, 2017

gijzelaerr commented Nov 26, 2017 • edited Loading

o-smirnov commented Apr 21, 2018

gijzelaerr commented Apr 23, 2018

o-smirnov commented Apr 23, 2018

gijzelaerr commented Apr 23, 2018

JSKenyon commented Nov 7, 2017 •

edited

Loading

gijzelaerr Nov 23, 2017 •

edited

Loading

gijzelaerr Nov 23, 2017 •

edited

Loading

sjperkins Nov 23, 2017 •

edited

Loading

JSKenyon commented Nov 23, 2017 •

edited

Loading

JSKenyon commented Nov 23, 2017 •

edited

Loading

gijzelaerr commented Nov 23, 2017 •

edited

Loading

sjperkins commented Nov 24, 2017 •

edited

Loading

gijzelaerr commented Nov 26, 2017 •

edited

Loading