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

Use setuptools in setup.py #231

Merged
merged 3 commits into from
Feb 18, 2019
Merged

Use setuptools in setup.py #231

merged 3 commits into from
Feb 18, 2019

Conversation

mbjoseph
Copy link
Member

@mbjoseph mbjoseph commented Feb 14, 2019

This PR removes the installation dependency on numpy's distutils module, and instead uses setuptools as recommended by the PSF: https://packaging.python.org/guides/tool-recommendations/

The use of numpy's distutils module seems to be unnecessary for earthpy, because it does not use the specialized features in numpy.distutils (e.g., the ability to handle Fortan source files as described here: https://docs.scipy.org/doc/numpy-1.14.0/f2py/distutils.html)

Locally, this solved #206 for me. I'm opening this as a draft to see how the builds shake out on Travis. I also would like to have other folks (@lwasser and @joemcglinchy if you have time) test this on Mac and Windows if possible, to make sure that the installation still proceeds as expected given the changes to setup.py.

@lwasser
Copy link

lwasser commented Feb 14, 2019

hey @mbjoseph some of these things impacts the RTD build. So one thing to check is whether the branch builds in RTD before we review this. If it builds that is great!!! :) i just remember fighting with it.

@mbjoseph
Copy link
Member Author

Good call @lwasser!

@codecov-io
Copy link

codecov-io commented Feb 14, 2019

Codecov Report

Merging #231 into master will increase coverage by 1.31%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #231      +/-   ##
==========================================
+ Coverage   91.24%   92.56%   +1.31%     
==========================================
  Files          14       15       +1     
  Lines         925     1116     +191     
==========================================
+ Hits          844     1033     +189     
- Misses         81       83       +2
Impacted Files Coverage Δ
earthpy/spatial.py 93.6% <0%> (-1.53%) ⬇️
earthpy/tests/test_plot.py 100% <0%> (ø) ⬆️
earthpy/tests/conftest.py 100% <0%> (ø) ⬆️
earthpy/tests/test_plot_rgb.py 100% <0%> (ø)
earthpy/plot.py 92.02% <0%> (+3.64%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce82643...f881f8f. Read the comment docs.

@mbjoseph mbjoseph changed the title Use distutils from the standard library in setup.py Use setuptools in setup.py Feb 14, 2019
@mbjoseph
Copy link
Member Author

Ok @lwasser I've checked RTD on this branch and the docs seem to be building correctly - have a look: https://earthpy.readthedocs.io/en/use-distutils/ (note this is a protected build so I'm not sure it will be visible to others).

I would love if both @lwasser and @joemcglinchy could test this branch locally on Mac and Windows (respectively), and confirm that our installation instructions lead to a successful install, passing tests, etc. for a fresh earthpy-dev conda environment.

@mbjoseph mbjoseph marked this pull request as ready for review February 15, 2019 01:06
@mbjoseph mbjoseph added documentation documentation added or edited in earthpy review requested labels Feb 15, 2019
@lwasser
Copy link

lwasser commented Feb 16, 2019

dear @mbjoseph this worked to install earthpy with pip version 19.0.2 !! magic... @joemcglinchy can you try this on windows?? i suspect this will be a great fix.

@lwasser
Copy link

lwasser commented Feb 18, 2019

hey @mbjoseph just a note that I just installed and got this error (i'm NOT using the earthpy-dev env right now btw). just did a test on a fresh windows VM conda envt. i'll try with the envt next.

$ pip install -e .
Obtaining file:///C:/Users/lewa8222/My%20Documents/github/earthpy
  Installing build dependencies: started
  Installing build dependencies: finished with status 'done'
Requirement already satisfied: tqdm in c:\users\lewa8222\anaconda3\lib\site-packages (from earthpy===VERSION-0.6.0-) (4.26.0)
Requirement already satisfied: pandas in c:\users\lewa8222\anaconda3\lib\site-packages (from earthpy===VERSION-0.6.0-) (0.23.4)
Requirement already satisfied: numpy in c:\users\lewa8222\anaconda3\lib\site-packages (from earthpy===VERSION-0.6.0-) (1.15.1)
Collecting geopandas (from earthpy===VERSION-0.6.0-)
  Downloading https://files.pythonhosted.org/packages/24/11/d77c157c16909bd77557d00798b05a5b6615ed60acb5900fbe6a65d35e93/geopandas-0.4.0-py2.py3-none-any.whl (899kB)
Requirement already satisfied: matplotlib>=2.0.0 in c:\users\lewa8222\anaconda3\lib\site-packages (from earthpy===VERSION-0.6.0-) (2.2.3)
Collecting rasterio (from earthpy===VERSION-0.6.0-)
  Downloading https://files.pythonhosted.org/packages/4e/79/b3e2d1783cde76d31eb87fa859eadf6855743d18c52942fbaa5f638e98a9/rasterio-1.0.18.tar.gz (1.8MB)
    Complete output from command python setup.py egg_info:
    INFO:root:Building on Windows requires extra options to setup.py to locate needed GDAL files. More information is available in the README.
    ERROR: A GDAL API version must be specified. Provide a path to gdal-config using a GDAL_CONFIG environment variable or use a GDAL_VERSION environment variable.

    ----------------------------------------
Command "python setup.py egg_info" failed with error code 1 in C:\Users\lewa8222\AppData\Local\Temp\pip-install-30zuipzh\rasterio\
You are using pip version 10.0.1, however version 19.0.2 is available.
You should consider upgrading via the 'python -m pip install --upgrade pip' command.

curious if @joemcglinchy gets a similar issue or not since I rarely use windows.

UPDATE: it installed perfectly once i had the Ea-envt ACTIVATED. probably because gdal is there and happily setup. but it's potentially problematic if it doesn't install without that envt. i wonder if we need to add more dependencies to earthpy like gdal?? thoughts? let's see what joe comes back with.

@mbjoseph
Copy link
Member Author

Thanks @lwasser - the intention here is to still use the conda environment. Glad it worked with the earthpy-dev conda environment!

@lwasser
Copy link

lwasser commented Feb 18, 2019

ahhh i need to try it with the dev envt next @mbjoseph let me do that. i just used the full envt ! not sure why i did that - i think just being lazy as it was already installed... more in a bit

@joemcglinchy
Copy link
Member

i tried installing from environment.yml with no conda environment activated. That installed fine, but upon activating and running pip install e . I receive the following:

(earthpy-dev-wintest2) C:\Projects\earth_py_support\joe_fork_github\earthpy>pip install e .
Processing c:\projects\earth_py_support\joe_fork_github\earthpy
  Installing build dependencies ... done
  Getting requirements to build wheel ... error
  Complete output from command C:\software\Anaconda3\envs\earthpy-dev-wintest2\python.exe C:\software\Anaconda3\envs\earthpy-dev-wintest2\lib\site-packages\pip\_vendor\pep517\_in_process.py get_requires_for_build_wheel C:\Users\jomc9287\AppData\Local\Temp\tmptyg2duiy:
  Traceback (most recent call last):
    File "C:\software\Anaconda3\envs\earthpy-dev-wintest2\lib\site-packages\pip\_vendor\pep517\_in_process.py", line 207, in <module>
      main()
    File "C:\software\Anaconda3\envs\earthpy-dev-wintest2\lib\site-packages\pip\_vendor\pep517\_in_process.py", line 197, in main
      json_out['return_val'] = hook(**hook_input['kwargs'])
    File "C:\software\Anaconda3\envs\earthpy-dev-wintest2\lib\site-packages\pip\_vendor\pep517\_in_process.py", line 54, in get_requires_for_build_wheel
      return hook(config_settings)
    File "C:\Users\jomc9287\AppData\Local\Temp\pip-build-env-e7xa6ql3\overlay\Lib\site-packages\setuptools\build_meta.py", line 130, in get_requires_for_build_wheel
      return self._get_build_requires(config_settings, requirements=['wheel'])
    File "C:\Users\jomc9287\AppData\Local\Temp\pip-build-env-e7xa6ql3\overlay\Lib\site-packages\setuptools\build_meta.py", line 112, in _get_build_requires
      self.run_setup()
    File "C:\Users\jomc9287\AppData\Local\Temp\pip-build-env-e7xa6ql3\overlay\Lib\site-packages\setuptools\build_meta.py", line 211, in run_setup
      self).run_setup(setup_script=setup_script)
    File "C:\Users\jomc9287\AppData\Local\Temp\pip-build-env-e7xa6ql3\overlay\Lib\site-packages\setuptools\build_meta.py", line 126, in run_setup
      exec(compile(code, __file__, 'exec'), locals())
    File "setup.py", line 3, in <module>
      from numpy.distutils.core import setup
  ModuleNotFoundError: No module named 'numpy'

@mbjoseph
Copy link
Member Author

@joemcglinchy are you sure you are on this branch? The error you got is a result of code that this PR removes.

@joemcglinchy
Copy link
Member

nope let me update and try again

@joemcglinchy
Copy link
Member

if you read my previous comment, disregard (i deleted it, forgot to activate my dev env!).

pip install e . completes, as do the subsequent commands.

@mbjoseph
Copy link
Member Author

Ok @lwasser @joemcglinchy thank you both for testing this locally. I have also tested this on Windows via AppVeyor and on Travis and it all seems to be working correctly.

This appears to solve #206 for us (across platforms too, which is great). I'll let @lwasser approve and merge when ready.

@lwasser
Copy link

lwasser commented Feb 18, 2019

@mbjoseph as a followup i just reinstalled on windows and it works great using the earthpy-dev envt !! so i second @joemcglinchy comment above ... this might be good to go now?? yea?

Copy link

@lwasser lwasser left a comment

Choose a reason for hiding this comment

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

i think this is good to go @mbjoseph but please let me know what you think before i merge. i have tested it and it seems to work well.

@mbjoseph
Copy link
Member Author

Cool - thanks for checking this again @lwasser! This should be good to merge now.

@lwasser
Copy link

lwasser commented Feb 18, 2019

woo hoo!! merging. thank you @mbjoseph !!

@lwasser lwasser merged commit 3ae84b7 into master Feb 18, 2019
@lwasser lwasser deleted the use-distutils branch June 18, 2020 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation documentation added or edited in earthpy review requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants