search

mila-iqia / fuel

A data pipeline framework for machine learning
MIT License
867 stars 268 forks source link

Fuel master is broken #261

Closed rizar closed 8 years ago

rizar commented 9 years ago

Restarting the tests broke the master branch.

dwf commented 9 years ago

I saw this this morning. What exactly is gone wrong?

bartvm commented 9 years ago

Just something I noticed: The errors seem to suggest that the current working directory doesn't exist. That would suggest the code changed directory somewhere, but never changed back. A search for chdir turns up this. My guess is that it crashes somewhere in those lines, leaving Python in the temporary directory that gets deleted later on. From thereon, trying to open any file results in an error.

rizar commented 9 years ago

I guess it is the failure in tests.test_converters.TestAdult.test_convert that caused the incorrect current directory.

rizar commented 9 years ago

Bug also happens locally on my machine. @hantek , if was your pull request, can you please take a look why Adult dataset is broken now?

hantek commented 9 years ago

Yes I will check on that

hantek commented 9 years ago

I find it out.

In the tests we try to download adult dataset from its original website (https://archive.ics.uci.edu/ml/machine-learning-databases/); but now that site is down. The downloader downloads a message saying something like "Permission denied," but for sure it is not smart enough to find itself tricked... Then the converter tries to convert that error message into a dataset, which ends up with an unexpected error. The test program stucks there, and starts the rest of tests...

hantek commented 9 years ago

Is there substitutes for that dataset link? Or we change the downloader to download a file into some directory without changing the current working directory (so that one test fails doesn't influence the others)?

rizar commented 9 years ago

What about adding a bit of exception handling to make sure that in the event of failure we end up in the same directory where we started? Also, it would be great to see a more informative error in such cases, because the current one did not suggest at all that we could not download...

On 28 October 2015 at 15:47, Zhouhan LIN notifications@github.com wrote:

Is there substitutes for that dataset? Or we change the downloader to download a file into some directory without changing the current working directory (so that one test fails doesn't influence the others)?

— Reply to this email directly or view it on GitHub https://github.com/mila-udem/fuel/issues/261#issuecomment-151969451.

bartvm commented 9 years ago

Maybe something like this in utils: http://stackoverflow.com/a/169112

On Thu, Oct 29, 2015 at 8:58 AM, Dzmitry Bahdanau notifications@github.com wrote:

What about adding a bit of exception handling to make sure that in the event of failure we end up in the same directory where we started? Also, it would be great to see a more informative error in such cases, because the current one did not suggest at all that we could not download...

On 28 October 2015 at 15:47, Zhouhan LIN notifications@github.com wrote:

Is there substitutes for that dataset? Or we change the downloader to download a file into some directory without changing the current working directory (so that one test fails doesn't influence the others)?

— Reply to this email directly or view it on GitHub https://github.com/mila-udem/fuel/issues/261#issuecomment-151969451.

— Reply to this email directly or view it on GitHub https://github.com/mila-udem/fuel/issues/261#issuecomment-152172763.

rizar commented 9 years ago

Sounds good to me. @vdumoulin , will you have time in the near future to fix this problem?

hantek commented 9 years ago

I will take care of it this night.

hantek commented 9 years ago

Hi, I created a PR at #265 to solve the problem. The goal of this PR is to make the test program remain at the right working dir even if any single test fails. But the tests on Adult dataset would still fail because the site is offline now.

hantek commented 9 years ago

Oh that site is back! It has passed!

dwf commented 8 years ago

Should this have been closed?

bartvm commented 8 years ago

Yeah. That said, I think a context manager like in #265 would still be good to merge. I believe there are several tests that could make the same thing happen i.e. if one test fails and doesn't change it's working directory back all the subsequent tests fail as well.