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

Make pyshp and pyproj external dependencies #234

Merged
merged 26 commits into from
Dec 7, 2015
Merged

Make pyshp and pyproj external dependencies #234

merged 26 commits into from
Dec 7, 2015

Conversation

micahcochran
Copy link
Contributor

This is in response to Issue #230.

This patch makes pyshp and pyproj packages external dependencies. It most removes shapefile.py and pyproj.py. It still maintains import locations for pyproj and shapefile, so code such as from mpl_toolkits.basemap.shapefile import Reader should still work.

I have NOT tested this. The unitesting in test.py does not make use of shapefiles, and I'm unsure if it will catch a problems with pyproj. There very will could have been something that I've missed. For the moment, this might need to go in a separate branch.

Installation will be a little harder due to the new dependencies.

src folder with PROJ4 has been left untouched.

"from pyproj import *" for namespace import compatibility
leaves namespace compatibility "from shapefile import *"
Changes the location to import the packages to the python system-wide package location.
removes licensing info about pyshp
mostly for Tavis-CI, but is also useful for pip
Removed sections for building PROJ.4.

Using a different (clearer?) way to check sys.version_info for python version 2.4 or greater.
@@ -32,6 +32,7 @@ install:
pip install --upgrade setuptools
pip install $NUMPY
pip install $MPL
pip install -r requirements.txt
- |
cd geos-3.3.3
export GEOS_DIR=$HOME/.local/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the GEOS stuff can be eliminated? Wasn't only for pyproj? (I could be wrong, though).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is failing because I removed the code from setup.py to compile _geoslib.c which is for GEOS library. Oops.

At first glance, shapely looks like it could be replaced GEOS. Also, that would put the burden of installing GEOS onto shapely package.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I suspect we don't need to compile geos anymore. So try taking that
out of the travis yaml. As for shapely, there are definite benefits to
updating the codebase to depend on that very nice package, but I think that
is orthogonal to the problem at hand.

On Tue, Dec 1, 2015 at 1:18 PM, Micah Cochran notifications@github.com
wrote:

In .travis.yml
#234 (comment):

@@ -32,6 +32,7 @@ install:
pip install --upgrade setuptools
pip install $NUMPY
pip install $MPL

  • pip install -r requirements.txt
  • |
    cd geos-3.3.3
    export GEOS_DIR=$HOME/.local/

This is failing because I removed the code from setup.py to compile
_geoslib.c which is for GEOS library. Oops.

At first glance, shapely http://toblerity.org/shapely/manual.html looks
like it could be replaced GEOS. Also, that would put the burden of
installing GEOS onto shapely package.


Reply to this email directly or view it on GitHub
https://github.com/matplotlib/basemap/pull/234/files#r46316025.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume compiling GEOS isn't a huge issue.

Migrating to shapely should be done at a later point in time.

Adding lines back to setup.py I removed too many lines which caused _geoslib.c to not compile.
@WeatherGod
Copy link
Member

Hmm, looks like the import of pyproj is still getting messed up somehow. The pyproj that becomes available is essentially empty, causing the attribute errors Travis is seeing.

Cython wrapper to provide python interfaces to
PROJ.4 (http://trac.osgeo.org/proj/) functions.
# pyproj compatibility layer for externally hosted package
# pyproj development occurs at https://github.com/jswhit/pyproj
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file probably needs to be eliminated entirely. It is getting picked up by the pyproj imports the other files are doing because it is in the same directory.

The basemaps local pyproj.py is causing namespace import issues.
@WeatherGod
Copy link
Member

Progress... but it looks like we have angered the py3k gods somehow. Nothing obvious to me at the moment.

@micahcochran
Copy link
Contributor Author

From my local testing I think it is pyproj. I tested this on Python 3.4.3. I installed pyproj 1.9.3 and 1.9.4 and both produced two errors on the same tests (test_convert and test_results_should_be_same_for_c_and_f_order_arrays).

test_results_should_be_same_for_c_and_f_order_arrays is very new to test for #212, which is pyproj4/pyproj#29 . That is only in the development version of pyproj. That is odd that it only impacts Python 3.

test_convert is also very new test.

I installed the development version (and added an import sys, which is a current PR), and there were no errors.

The development version of pyproj plays nicely with Python 3.x.

Here's where pyproj is at at the moment. There is currently an PR to add Travis-CI to pyproj. In a patch that I sent I forgot an import sys.
I will ask for the development version pyproj to be runnable, but a new pyproj release is likely needed. Using the development version can cause unexpected problems like this.

@WeatherGod
Copy link
Member

Ok, so to help guide decision-making here, let's settle some questions. First, is there a work-around for this bug we are encountering? If not, which versions of pyproj are we going to have to blacklist?

Tangentially, this section of code is ancient and is causing a number of problems elsewhere. The reason for its existence was for back in the numeric/numarray days. Perhaps it makes sense to rip this stuff out (separately, of course)?

@micahcochran
Copy link
Contributor Author

I should revise my statement above: both failing tests are from PR #223, which patch issue #209. pyproj patched this same issue in pyproj4/pyproj#29 .

There could be a minimal version of pyproj, perhaps 1.9.3, for Python 2.x. The minimum version pyproj for Python 3.x should probably the next release.

It is possible to skip the two (2) failing unit tests for Python 3 using current version of pyproj . Doing something like this in test.py:

import sys
PY3 = (sys.version_info[0] == 3)
  ...
@unittest.skipIf( PY3 and pyproj.__version__ <= "1.9.4", 
                  "TestProjectCoords tests skipped for pyproj version 1.9.4 for Python 3.x" )
class TestProjectCoords(TestCases):
   ...

That will cause it to skip the failing tests. IMO it is slightly unfair for an unpatched library to be expected pass new unit tests.

I see two courses of action. (1.) Add the code above to skip the tests (my preference). (2.) Wait for pyproj to get the development version working, install pyproj using the development version, but that means that basemap will probably should wait for a new version of pyproj to be released before it can have a new release.

I agree with getting rid of the old numeric/numarray code if it causes problems elsewhere.

Skips tests in TestProjectCoords only when using Python 3.x and pyproj 1.9.4 or below.  The patches for these problems are in the current development version of pyproj.
nose is required for the numpy.test.decorators.skipif() function used in the test cases in test.py.
Removed skipif decorator (it may be buggy or I might not understand it).  Used a SkipTest Exception.  Seems to work in local testing of Python 3.4.
Also, imports backport of unittest (unittest2) for @skipIf decorator.  This is needed for Python 2.4 to 2.6.

I prefer this syntax over raise SkipTest("skipped the test message").
This file is causing namespace issues.
@micahcochran
Copy link
Contributor Author

I think that I've got this working per the unit tests.

The syntax from mpl_toolkit.basemap import pyproj still works because there is an import pyproj statement in init.py. import mpl_toolkit.basemap.pyproj does not work.

Currently, from mpl_toolkit.basemap import shapefile does not work. The could easily be added for some backward compatibility, if desired. Let me know if you want me to make that change.

@WeatherGod Thank you for all your help with this. Sorry for gridlocking the project for about a week.

@micahcochran
Copy link
Contributor Author

I just thought of this; pyshp could easily be made an optional dependency. It is required for .readshapefile() and .drawcounties(). Only three four of of the examples need it: fillstates.py, hurrtracks.py, counties.py, and plotcities.py.

@@ -1,7 +1,6 @@
import sys, glob, os, subprocess

major, minor1, minor2, s, tmp = sys.version_info
if major==2 and minor1<4 or major<2:
if sys.version_info < (2, 4):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be fine bumping the minimum version to at least 2.6 which is the oldest` one that we test with. Matplotlib has dropped support for 2.6 and earlier now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just trying to make the version requirements clearer without having all of those extra variables. A minimum version of 2.6 is reasonable Its last security fix, 2.6.9, was on October 29, 2013.

I'll modify that line to 2.6.

@jenshnielsen
Copy link
Member

👍 looks good to me

Making pyshp optional makes sense but that could just as easily be done in a different PR

@WeatherGod
Copy link
Member

I am -1 on making pyshp optional at this point. I know I have used it
myself when I didn't want the bulkiness of GDAL, and I have seen others use
it, too. I don't think there would be much to gain by making it optional,
versus the confusion it could cause.

I just remembered that there is a hidden optional dependency in basemap:
OWSLib for wmsimage(). Obviously, it isn't tested.

On Mon, Dec 7, 2015 at 12:26 PM, Jens Hedegaard Nielsen <
notifications@github.com> wrote:

[image: 👍] looks good to me

Making pyshp optional makes sense but that could just as easily be done in
a different PR


Reply to this email directly or view it on GitHub
#234 (comment).

library_dirs=geos_library_dirs,
runtime_library_dirs=runtime_lib_dirs,
include_dirs=geos_include_dirs,
libraries=['geos_c']) ]

# Specify all the required mpl data
# create pyproj binary datum shift grid files.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. I'll remove that comment.

@micahcochran
Copy link
Contributor Author

Since pyshp is a python only library, it is so easy to install. I don't see it being a problem to require it. I thought I would bring it up.

Edit: Typo.

@jenshnielsen
Copy link
Member

👍 Will wait for @WeatherGod to merge

WeatherGod added a commit that referenced this pull request Dec 7, 2015
Make pyshp and pyproj external dependencies
@WeatherGod WeatherGod merged commit 6497e42 into matplotlib:master Dec 7, 2015
@WeatherGod
Copy link
Member

Great work, @micahcochran! We will have to see about unbundling libgeos completely, but at least there is existing pathways for distributors to package it without the old geos codebase.

@micahcochran
Copy link
Contributor Author

@WeatherGod Thanks for all your help.

@jdkloe
Copy link

jdkloe commented Dec 8, 2015

thanks for adopting the idea to remove these dependencies from the code base.

This was referenced Dec 8, 2015
@micahcochran micahcochran deleted the ext-pyshp-pyproj branch December 9, 2015 17:12
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.

4 participants