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

Always use cythonized files in sdist. #1264

Merged
merged 6 commits into from
Jan 16, 2019
Merged

Conversation

pelson
Copy link
Member

@pelson pelson commented Jan 14, 2019

Rationale

In #1258 @scoder suggested that we should consider making recompilation of pyx files optional. I can see some pros/cons to this approach, but it does mean that Cython is removed as an installation dependency, and would close #1258. The approach I've taken mimics numpy's - it only recompiles the Cythonized c++/c if we are not in a sdist (i.e. there is no PKG-INFO).

Implications

  • Changes to the pyx files will be ignored when running setup.py IFF there is a PKG-INFO file at the same level as the setup.py (no for github source, yes for sdist).
  • sdist now need to ship the Cythonized files as well as the pyx.

QuLogic
QuLogic previously approved these changes Jan 15, 2019
Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

How do we ensure that the Cythonized files are included in the sdist?

@QuLogic
Copy link
Member

QuLogic commented Jan 15, 2019

License header failures though.

@pelson
Copy link
Member Author

pelson commented Jan 15, 2019

How do we ensure that the Cythonized files are included in the sdist?

python setup.py sdist should build and include them, no?

If it didn't, the sdist would not be installable (as it would automatically replace source.pyx with source.cpp, but the file wouldn't exist).

Unfortunately this is having issues with py27 and the compilation step.

@QuLogic
Copy link
Member

QuLogic commented Jan 15, 2019

Ah yes, you're right about sdist.

It's not Python 2; it's Python 2 with old Cython and Python 3 with old-ish Cython (which seems to be triggering the error in #1253.)

@pelson
Copy link
Member Author

pelson commented Jan 15, 2019

It's not Python 2; it's Python 2 with old Cython and Python 3 with old-ish Cython (which seems to be triggering the error in #1253.)

But why with this change, and not before... must be something odd with the way we setup the environment in travis.

@pelson
Copy link
Member Author

pelson commented Jan 15, 2019

Interesting, a successful log has the following:

$ pip install --no-deps .
Processing /home/travis/build/SciTools/cartopy
  Installing build dependencies ... done

No further useful diagnostics, but I wonder if a newer version of cython is being installed even though Cython is already installed.

@QuLogic
Copy link
Member

QuLogic commented Jan 15, 2019

That's also there in the failing build, though. A diff on the conda list output doesn't seem too illuminating.

@pelson
Copy link
Member Author

pelson commented Jan 15, 2019

That's also there in the failing build, though.

But I removed Cython from pyproject.toml's build_system item.

@QuLogic
Copy link
Member

QuLogic commented Jan 15, 2019

Ah right.

The CRON job also started failing yesterday and I thought it was the same error, but it was actually the NumPy ufunc thing.

.travis.yml Outdated
- PYTHON_VERSION=3.5
PACKAGES="cython=0.23.1 numpy=1.10.0 matplotlib=1.5.1 nose proj4=4.9.1 scipy=0.16.0 libgfortran=1"
PACKAGES="cython=0.* numpy=1.10.0 matplotlib=1.5.1 nose proj4=4.9.1 scipy=0.16.0 libgfortran=1"
Copy link
Member Author

Choose a reason for hiding this comment

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

Future-proofing for Cython 3.

@QuLogic
Copy link
Member

QuLogic commented Jan 15, 2019

I think we need to pass --no-build-isolation so it doesn't try to install new build deps. This might also fix the ufunc thing if we were also building against a newer numpy than is actually installed?

@pelson
Copy link
Member Author

pelson commented Jan 15, 2019

I think we need to pass --no-build-isolation so it doesn't try to install new build deps. This might also fix the ufunc thing if we were also building against a newer numpy than is actually installed?

Confirmed. An example log of pip installing build dependencies, even if they are installed (with decent stdout to see what is going on): https://circleci.com/gh/SciTools/cartopy/1277?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Is there an issue somewhere in pip? scipy/scipy#8734 looks somewhat related. IMO It is quite unexpected that pip should re-install the build dependencies, even if they are already installed.

…installs packages that are already installed.

For example, if numpy is in [build-system]->requires, and numpy is *already installed* in the environment that is running pip,
the *existing* version of numpy should be used to build cartopy, **not** the latest version found on pypi.
@pelson
Copy link
Member Author

pelson commented Jan 15, 2019

I just added a commit with the following description:

pyproject.toml was removed. It can be returned once pip no longer re-installs packages that are already installed. For example, if numpy is in [build-system]->requires, and numpy is already installed in the environment that is running pip, the existing version of numpy should be used to build cartopy, not the latest version found on pypi.

This should serve as a reference should we get future PRs adding a pyproject.toml - the situation must change with pip before cartopy will accept a pyproject.toml again. I fully recognise that this means that users who pip install cartopy will error out unless they pip install numpy before-hand. Unfortunately this is the state of the python packaging tooling, and is not something we can work around.

@QuLogic
Copy link
Member

QuLogic commented Jan 15, 2019

This should work to support 0.28 (as it seems there's no 0.29 for Python 3.5):

diff --git a/lib/cartopy/trace.pyx b/lib/cartopy/trace.pyx
index c1d8d5a..065946a 100644
--- a/lib/cartopy/trace.pyx
+++ b/lib/cartopy/trace.pyx
@@ -170,7 +170,7 @@ cdef class LineAccumulator:
                                                &geoms[0], geoms.size())
         return geom
 
-    cdef list[Line].size_type size(self):
+    cdef size_t size(self):
         return self.lines.size()

@pelson
Copy link
Member Author

pelson commented Jan 15, 2019

AppVeyor is queued, yet passed on the new SciTools one at https://ci.appveyor.com/project/SciTools/cartopy/builds/21631645/job/w1m0vd7bd74eo1ry.

@pelson
Copy link
Member Author

pelson commented Jan 16, 2019

@QuLogic - would you mind merging when happy with the changes? I don't think the AppVeyor results are going to come back (I've started fixing #1221 and am afraid I've caused this to happen accidentally).

With this merged we unblock the CI, have Cython 0.28 compatibility, and avoid needing Cython for users to install the sdist from pypi. 👍

@QuLogic
Copy link
Member

QuLogic commented Jan 16, 2019

I would keep the pins on Travis and AppVeyor, as I specifically put them in there to ensure it continued to work (even if that ended up being broken...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cartopy cannot be built on a FIPS-enabled system
2 participants