search

developmentseed / landsat-util

A utility to search, download and process Landsat 8 satellite imagery
Creative Commons Zero v1.0 Universal
689 stars 147 forks source link

Loosen requirement versions #236

Closed astrojuanlu closed 7 years ago

astrojuanlu commented 7 years ago

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.

astrojuanlu commented 7 years ago

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

matthewhanson commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

astrojuanlu commented 7 years ago

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 commented 7 years ago

What's the status on this?

astrojuanlu commented 7 years ago

Ready to merge!

On Aug 24, 2017 21:35, "Waqy" notifications@github.com wrote:

What's the status on this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/developmentseed/landsat-util/pull/236#issuecomment-324735231, or mute the thread https://github.com/notifications/unsubscribe-auth/AATUZeEpHi81L-53_YnI0Uzb79IZWsOIks5sbdCdgaJpZM4OciSu .

astrojuanlu commented 6 years ago

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

guillochon commented 6 years ago

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