search

lvapeab / nmt-keras

Neural Machine Translation with Keras
http://nmt-keras.readthedocs.io
MIT License
532 stars 130 forks source link

Add setup.py #115

Closed davidwilby closed 4 years ago

davidwilby commented 4 years ago

This PR will:

However there is a problem with installing coco-caption as part of running python setup.py install which I haven't been able to track down yet (hence the draft PR). @lvapeab do you have any ideas? Basically during install of nmt-keras via setup.py then all the packages install from their intended locations except coco-caption (via coco-caption @ https://github.com/lvapeab/coco-caption/archive/master.zip) for some reason. But running 

pip install coco-caption @ https://github.com/lvapeab/coco-caption/archive/master.zip

works, and the other PEP508 URL dependencies (namely keras_wrapper and MarcBS' fork of keras) work absolutely fine.

Have also tried, cloning coco-caption and running python setup.py install which works too.

So I have no clue why including it in nmt-keras' setup.py breaks it..

lvapeab commented 4 years ago

Hi @davidwilby thank you so much for this PR! Let's dig into the problems!

I've just updated the Pypi package of multimodal_keras_wrapper, so it should be safe to just use this version.

I have been unable to install MarcBS' version of Keras directly from your setup.py. I had to list it via dependency_links. And the same with coco-caption. Doing this, it works.

I edited the PR. Please, check that it works also for you.

If it does, we can merge it! :)

davidwilby commented 4 years ago

Great news about keras wrapper!

What version of setuptools and pip are you using? dependency_links has been deprecated in setuptools as of version 41.5 (https://setuptools.readthedocs.io/en/latest/history.html#v41-5-0) due to support being dropped in pip from version 19. (which is a bit of a headache really)

Therefore, non PyPI dependencies should be specified via PEP508 URLs now, such as:

keras @ https://github.com/MarcBS/keras/archive/master.zip

I see in your commit that you've updated the install_requires to use a different version of coco-caption - does that mean that your fork (github.com/lvapeab/coco-caption) is not required? Or does it mean that the install of nmt-keras would depend on an existing local install of the coco-caption package?

lvapeab commented 4 years ago

Woops! Yes, my bad. I started a virtualenv and forgot to update pip.

So, following your setup.py with the correct way of specifying non-PyPI dependencies, I'm also unable to install them using python setup.py install. But it works if I install it directly using pip: pip install .. So, I guess we can go this way. Do you have additional thoughts before I merge it?

davidwilby commented 4 years ago

@lvapeab I've had a bit of a hack around with this and come to the same conclusion, that pip install . works but python setup.py install doesn't for coco-caption in this PEP508 URL form.

I can't get it to work at all, maybe there's something wrong in the coco-caption setup.py perhaps.

I suggest you just go ahead and merge this, thanks for working with me here!

lvapeab commented 4 years ago

Just note that python setup.py install doesn't work for Keras either. It doesn't crash because it takes the Keras version from PyPI, instead of the one from https://github.com/MarcBS/keras. In the case of coco-caption, it is unable to find the package in PyPI and crashes.

Anyway, I'll keep an eye on this and I'll revisit it in a future.

Thanks again for the PR and the suggestions!

davidwilby commented 4 years ago

Ah interesting, good to know, thanks!

davidwilby commented 4 years ago

Just to add some more info to this as I discover it: If 'coco-caption @ https://github.com/lvapeab/coco-caption/archive/master.zip' is in the setup.py and this is used to build a wheel, then you pip install blah.whl then this also works. So it seems that pip handles this format fine, but setuptools might be what has an issue with it.