search

LxMLS / lxmls-toolkit

Machine Learning applied to Natural Language Processing Toolkit used in the Lisbon Machine Learning Summer School
Other
222 stars 216 forks source link

Toolkit still does not pass Travis tests #135

Closed ramon-astudillo closed 5 years ago

ramon-astudillo commented 5 years ago

@kepler solved one of the errors https://github.com/LxMLS/lxmls-toolkit/issues/128

there are other errors in the deep learning day. Maybe consequence of the RL day updates

https://travis-ci.org/LxMLS/lxmls-toolkit/jobs/543686960

ramon-astudillo commented 5 years ago

@pschydlo found one error https://github.com/LxMLS/lxmls-toolkit/pull/136 though problem persists

https://travis-ci.org/LxMLS/lxmls-toolkit/builds/544238058?utm_source=github_status&utm_medium=notification

kepler commented 5 years ago

There are two problems causing the failed CI issue:

  1. PyTorch 0.3.1 was in the requirements, but this version doesn't really contain torch.distributions.categorical.
  2. With the requirements updated to PyTorch 0.4.0 and then 1.0.0, the problem remained because a torch==0.3.1 dependency was also specified inside tox.ini, thus failing the tests.

For reasons I haven't investigated further, running tests locally with tox does not cause this error, because tox install torch 0.4.0. However, if I have torch 0.3.1, install pytest, and then run pytest tests/test_sequence_models_deep_learning.py, I get the exact same error as in TravisCI.

These are the fixes I propose (I'll submit a PR soon):

  1. Make tox use dependencies from requirements.txt, removing redundant dependencies specifications (which led to this problem).
  2. The required PyTorch version is already increased, so importing Categorical will work, but a way that would have worked for 0.3.1 and still works is using from torch.distributions import Categorical instead of from torch.distributions.categorical import Categorical. This can also be done to rollback the "ugly" fix @ramon-astudillo mentions in #137.
kepler commented 5 years ago

OK, it's even worse. .travis.yml holds yet another dependency on torch 0.3.1, and it directly calls pytest instead of tox. I'm going to fix it and push a new commit to #137.