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

Loosen requirement versions #236

Merged

Conversation

astrojuanlu
Copy link
Contributor

I essentially widened the allowed versions for all dependencies, taking care of specifying rasterio<1. The current setup has been tested in all Python >= 3.4 versions (see commit logs). I also updated the Dockerfile, hopefully fixing another issue.

Tested in:

* Python 3.5 and 3.6 conda environments with latest
  NumPy and GDAL
* Official Python 3.4, 3.5 and 3.6 Docker images
  based on Debian Jessie using only pip
@astrojuanlu
Copy link
Contributor Author

(Issue links won't work in titles, so I hope this fixes #223).

@astrojuanlu astrojuanlu changed the title Loosen requirements, fix #223 Loosen requirement versions Jul 19, 2017
@matthewhanson
Copy link
Contributor

Thanks @Juanlu001 I think this would resolve #223, but I'm not sure it's such a good idea to use the minimum version syntax (>=) as it means that any major release for any of these packages (other than rasterio which you have specified) could break the install. The compatible release syntax (~=) I think might be preferable, e.g.

package~=1.1
is the same as saying >= 1.1. ==1.*

in the case of all these packages it doesn't make a difference right now, but could prevent a problem in the future.

I've built and run the new Docker file you included and things seem to work fine. I'm happy to go ahead and merge this but I'd like to give a chance to @ocefpaf to provide any feedback on how this addresses his concerns.

Also @drewbo @scisco if you have any strong feelings on version pinning please weight in.

Thanks again for the PR!

@ocefpaf
Copy link

ocefpaf commented Jul 20, 2017

I'd like to give a chance to @ocefpaf to provide any feedback on how this addresses his concerns.

LGTM. Thanks @Juanlu001 and @matthewhanson. This will make it much easier to build landsat-util.

@astrojuanlu
Copy link
Contributor Author

Before you rush and merge this, I'm actually OK with using compatible release markers. I will assume though that, if it worked with >=, it will work with ~= :)

@astrojuanlu
Copy link
Contributor Author

...on second thought: I think this deserves some testing still, but I won't be able to do it next week.

@astrojuanlu
Copy link
Contributor Author

I update the version requirements using ~= or putting explicit upper bounds where appropriate. I tested again with the official Python Docker images and everything seemed to work, so from my side this is ready to merge.

@waqarmalik
Copy link

What's the status on this?

@astrojuanlu
Copy link
Contributor Author

astrojuanlu commented Aug 24, 2017 via email

@matthewhanson matthewhanson merged commit e233f03 into developmentseed:develop Aug 24, 2017
@astrojuanlu astrojuanlu deleted the loosen-requirements branch August 24, 2017 20:35
@astrojuanlu
Copy link
Contributor Author

By the way, would it be possible to have a release with this changes? (A v0.14.0 I guess)

@guillochon
Copy link

Yes, can we please release this? The current version on pypi completely mucks with my Python stack.

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.

None yet

5 participants