coralnet / pyspacer

Python based tools for spatial image analysis
MIT License
6 stars 2 forks source link

unit-tests fail on master (#21) #22

Closed qiminchen closed 4 years ago

qiminchen commented 4 years ago
  1. change vgg16 features comparison tolerance
  2. remove the download of default efficientnet-b0
qiminchen commented 4 years ago

@beijbom I think I found where goes wrong, working on it right now

  1. the unit test failed at the TestCaffeExtractor when trying to compare the float32 vectors and expecting identical results, instead use a more robust to handle float precision errors. #21
  2. remove pretrained argument as pretrained refers to the pretrained weights on ImageNet which will never be used in our cases. #20

I'm looking at the refactor...

qiminchen commented 4 years ago

@beijbom I was able to pass the test cases at https://github.com/beijbom/pyspacer/commit/38144a6a8a79c2aaf500ddd3a7ba5757723e71e9 on docker as well, so

  1. I pull the commit https://github.com/beijbom/pyspacer/commit/38144a6a8a79c2aaf500ddd3a7ba5757723e71e9, refactored the extract_features.py but the test case failed on docker -> refactor might cause the test to break.
  2. I reverted the caffe_utils.py code back to https://github.com/beijbom/pyspacer/commit/38144a6a8a79c2aaf500ddd3a7ba5757723e71e9 for VGG16CaffeExtractor so no refractor was used but the test case still failed on docker -> something else cause the break.

I was able to solve this by setting the absolute tolerance atol=1e-5 for all the float32 vector comparison basically at https://github.com/beijbom/pyspacer/blob/ef85728cbaa8fb7c447125947240fe239d0851c6/spacer/tests/test_beta_regression.py#L141 https://github.com/beijbom/pyspacer/blob/ef85728cbaa8fb7c447125947240fe239d0851c6/spacer/tests/test_extract_features.py#L174-L176 https://github.com/beijbom/pyspacer/blob/ef85728cbaa8fb7c447125947240fe239d0851c6/spacer/tests/test_extract_features.py#L302-L305 and all the test cases passed on local, docker and Travis CI. I'm trying to find what causes the break, seems like it's not the refactor, what do you think?

beijbom commented 4 years ago

@beijbom I was able to pass the test cases at 38144a6 on docker as well, so

  1. I pull the commit 38144a6, refactored the extract_features.py but the test case failed on docker -> refactor might cause the test to break.
  2. I reverted the caffe_utils.py code back to 38144a6 for VGG16CaffeExtractor so no refractor was used but the test case still failed on docker -> something else cause the break.

Sorry, I don't understand this. So: 1) You ran the tests on https://github.com/beijbom/pyspacer/commit/38144a6a8a79c2aaf500ddd3a7ba5757723e71e9, and it passes. (I just checked, and it passes on my docker system also.) 2) Then you refactored the extraction code and the tests failed? 3) But then you reverted the code back to https://github.com/beijbom/pyspacer/commit/38144a6a8a79c2aaf500ddd3a7ba5757723e71e9 and then it still failed?

Do I understand that correctly? What was the difference between the code you ran in situation 1 and 3?

(Btw. CI won't catch this since CI doesn't run Docker and thus can't run caffe).

qiminchen commented 4 years ago

I went back and checked out aee4cad which was before the EfficientNet integration and refactor, and the test passes there.

@beijbom I built and test this commit https://github.com/beijbom/pyspacer/commit/aee4cad5f65399f708f2f5223b71a55123474a86 and the test passes, then I built and test the next commit https://github.com/beijbom/pyspacer/commit/7ffbc81e57bab6a30db668b1c4b0d3f0e0944144, the one I re-base to master that you merged and the test case failed, this one didn't have any refactor, would it be possible that some packages updating causes the break?

qiminchen commented 4 years ago
  1. You ran the tests on 38144a6, and it passes. (I just checked, and it passes on my docker system also.)
  2. Then you refactored the extraction code and the tests failed?

yes

  1. But then you reverted the code back to 38144a6 and then it still failed?

Do I understand that correctly? What was the difference between the code you ran in situation 1 and 3?

I only reverted the code for VGG16CaffeExtractor() in extract_features.py where VGG16CaffeExtractor() wasn't refactored at all and the test also failed.

Test passed at https://github.com/beijbom/pyspacer/commit/aee4cad5f65399f708f2f5223b71a55123474a86 but failed at the next commit https://github.com/beijbom/pyspacer/commit/7ffbc81e57bab6a30db668b1c4b0d3f0e0944144 where I re-based to master.

beijbom commented 4 years ago

I went back and checked out aee4cad which was before the EfficientNet integration and refactor, and the test passes there.

@beijbom I built and test this commit aee4cad and the test passes, then I built and test the next commit 7ffbc81, the one I re-base to master that you merged and the test case failed, this one didn't have any refactor, would it be possible that some packages updating causes the break?

Hmm. I just tried https://github.com/beijbom/pyspacer/commit/7ffbc81e57bab6a30db668b1c4b0d3f0e0944144 and that one still passes. That commit fails in your docker build?

beijbom commented 4 years ago

Man, this is confusing. For me, the break happens between ebb0bdb09ef043c87e6e787ca74ce18ad59c6504 and 7ffbc81e57bab6a30db668b1c4b0d3f0e0944144. Check out the diff below. Nothing there looks like it'd affect the vgg16 extractor.

https://github.com/beijbom/pyspacer/compare/7ffbc81e57bab6a30db668b1c4b0d3f0e0944144...ebb0bdb09ef043c87e6e787ca74ce18ad59c6504

beijbom commented 4 years ago

... I'll check in again tomorrow. If we haven't solved it by then I'm ready to give up and go with your solution by setting the test tolerance.

qiminchen commented 4 years ago

Hmm. I just tried 7ffbc81 and that one still passes. That commit fails in your docker build?

hmmmm it failed at my docker build.... I'm double-checking

beijbom commented 4 years ago

Hmm. I just tried 7ffbc81 and that one still passes. That commit fails in your docker build?

hmmmm it failed at my docker build.... I'm double-checking

@qiminchen : still no luck with this?

qiminchen commented 4 years ago

@qiminchen : still no luck with this?

@beijbom sadly no, it's so confusing...

beijbom commented 4 years ago

Ok. No worries. Let’s just go with your proposed solution. It’s not worth spending more time on.

On Mon, May 4, 2020 at 21:03 Qimin Chen notifications@github.com wrote:

@qiminchen https://github.com/qiminchen : still no luck with this?

@beijbom https://github.com/beijbom sadly no, it's so confusing...

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/beijbom/pyspacer/pull/22#issuecomment-623846408, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAITTFZOJCOXOXQTK6MJU63RP6F2DANCNFSM4MYHCQ2Q .

qiminchen commented 4 years ago

@beijbom btw is it normal to have these when running and testing in the docker? Tho the test cases pass.

-> Patching legacy classifier.
-> Patching legacy classifier.
.-> Extracting features for job:beta_reg_test.
-> Initializing VGG16CaffeExtractor
/usr/lib/python3.6/importlib/_bootstrap.py:219: RuntimeWarning: numpy.ufunc size changed, may indicate binary incompatibility. Expected 192 from C header, got 216 from PyObject
  return f(*args, **kwds)
/usr/lib/python3.6/importlib/_bootstrap_external.py:426: ImportWarning: Not importing directory /usr/local/lib/python3.6/dist-packages/google: missing __init__
  _warnings.warn(msg.format(portions[0]), ImportWarning)
/usr/lib/python3.6/importlib/_bootstrap_external.py:426: ImportWarning: Not importing directory /usr/local/lib/python3.6/dist-packages/mpl_toolkits: missing __init__
  _warnings.warn(msg.format(portions[0]), ImportWarning)
[libprotobuf WARNING google/protobuf/io/coded_stream.cc:604] Reading dangerously large protocol message.  If the message turns out to be larger than 2147483647 bytes, parsing will be halted for security reasons.  To increase the limit (or to disable these warnings), see CodedInputStream::SetTotalBytesLimit() in google/protobuf/io/coded_stream.h.
[libprotobuf WARNING google/protobuf/io/coded_stream.cc:81] The total number of bytes read was 552712164
.-> Extracting features for job:beta_reg_test.
-> Initializing VGG16CaffeExtractor
.[libprotobuf WARNING google/protobuf/io/coded_stream.cc:604] Reading dangerously large protocol message.  If the message turns out to be larger than 2147483647 bytes, parsing will be halted for security reasons.  To increase the limit (or to disable these warnings), see CodedInputStream::SetTotalBytesLimit() in google/protobuf/io/coded_stream.h.
[libprotobuf WARNING google/protobuf/io/coded_stream.cc:81] The total number of bytes read was 552712164
.............-> Initializing VGG16CaffeExtractor
.-> Initializing VGG16CaffeExtractor
----------------------------------------------------------------------
Ran 133 tests in 84.773s

OK (skipped=2)
Name                                 Stmts   Miss  Cover   Missing
------------------------------------------------------------------
spacer/__init__.py                       1      0   100%
spacer/caffe_utils.py                   42      0   100%
spacer/config.py                        54      0   100%
spacer/data_classes.py                  81      0   100%
spacer/extract_features.py              55      0   100%
spacer/extract_features_utils.py        28      0   100%
spacer/mailman.py                       15      0   100%
spacer/messages.py                     163      0   100%
spacer/models/__init__.py                6      0   100%
spacer/models/effcientnet_utils.py     126      0   100%
spacer/models/efficientnet.py          111      0   100%
spacer/storage.py                      127      0   100%
spacer/task_utils.py                    11      0   100%
spacer/tasks.py                         37      0   100%
spacer/torch_utils.py                   34      0   100%
spacer/train_classifier.py              23      0   100%
spacer/train_utils.py                  108      0   100%
------------------------------------------------------------------
TOTAL                                 1022      0   100%
beijbom commented 4 years ago

@qiminchen : yeah, those warnings are ok. So now the test pass on https://github.com/beijbom/pyspacer/commit/7ffbc81e57bab6a30db668b1c4b0d3f0e0944144?

beijbom commented 4 years ago

I give up. Let's move on with our lives. :)

qiminchen commented 4 years ago

@qiminchen : yeah, those warnings are ok. So now the test pass on 7ffbc81?

@beijbom Nope, that is the latest commit using the absolute tolerance, I cleaned the cache, delete the docker, reinstall the docker, pull the https://github.com/beijbom/pyspacer/commit/7ffbc81e57bab6a30db668b1c4b0d3f0e0944144, build image and run docker, still fail... :<