biolab / orange3-imageanalytics

🍊 :rice_scene: Orange3 add-on for dealing with image related tasks
GNU General Public License v3.0
32 stars 42 forks source link

Replace the Hyper library with HTTPX #162

Closed PrimozGodec closed 4 years ago

PrimozGodec commented 4 years ago
Issue

Fixes #146

Description of changes

With this pull request, we replace the buggy Hyper library with HTTPX. Server embedder is now implemented as an async client which means that it is not necessary to send images in batches anymore.

This implementation improves the speed of embedding. For example embedding times of 2949 YPLP:

The current implementation uses HTTP 1.1 since HTTPX still has bugs (https://github.com/encode/httpx/issues/551, https://github.com/encode/httpx/issues/514) in HTTP 2 that prevent us to use it. Once they fix bugs, we can move to HTTP 2 easily by changing one line of code.

@matjazp what do you think, should we merge this even it does not implement HTTP 2? The speed improvement is quite great. If we decide to merge it, we should fix HTTPX to ~=0.9.3 since the library is still in the alpha phase and some unexpected change can case troubles.

Before testing reinstall ImageAnalytics since new package must be installed

Includes
codecov[bot] commented 4 years ago

Codecov Report

Merging #162 into master will increase coverage by 2%. The diff coverage is 98.92%.

@@          Coverage Diff          @@
##           master   #162   +/-   ##
=====================================
+ Coverage   65.99%    68%   +2%     
=====================================
  Files          13     13           
  Lines        3238   3147   -91     
  Branches      482    451   -31     
=====================================
+ Hits         2137   2140    +3     
+ Misses        967    890   -77     
+ Partials      134    117   -17
ajdapretnar commented 4 years ago

I am getting a JSONDecodeError on openface. Errors are not cleared when a new embedder is selected and started.

PrimozGodec commented 4 years ago

@ajdapretnar error is fixed now.

PrimozGodec commented 4 years ago

Tests failing due to error in Orange https://github.com/biolab/orange3/pull/4320

PrimozGodec commented 4 years ago

@ajdapretnar made changes connected with the progress bar here. Can you test again when you will have time.

ajdapretnar commented 4 years ago

Things look ok now. Just one Q. When I disconnect the internet, with VGG for example, the embedder stalls (stays at the same progress). Should it not check the connection occasionally? Also, it looks like some embedders are cached locally. Is this so? When I am offline, I can still use the embedders I used before, but for new ones I get the 'Internet disconnected' error.

PrimozGodec commented 4 years ago

The behavior regarding the internet connection should be like this:

In most cases, the widget would be without connection in the beginning already and switch is fast. The connection can be lost during embedding (but this case is very rare). In this case, the waiting time of 30 seconds is not so long in my opinion since usually, it would happen between embedding of a large set of images which already takes a few minutes.

Before I was checking OSError which happens when sending images and there is no connection, now I also add checking ReadTimeout which usually happens when lost connection on slower embedders like VGG (it was not handled correctly before). So now widget should detect connection lost correctly. Widget always needs to switch to local embedder when no connection. If it does not do so it is a bug. In my case, everything works well now. @ajdapretnar can you please test again and confirm that it also works for you. It would be also good if it can be tested on Windows since there can be different behavior regarding OSError.

With this version, we do not ping server to check the connection anymore, since it is not nice and implementation can be more flexible. In case that embeddings are in the local cache embeddings can be calculated without connection.

ajdapretnar commented 4 years ago

Everything works well now, but can't merge due to failing tests.

ajdapretnar commented 4 years ago

p.s. I'll try on Windows, too.

PrimozGodec commented 4 years ago

Now I fixed:

PrimozGodec commented 4 years ago

I fixed tests and changed the strategy for embedding with different embedding. @ajdapretnar Can you test this again when you will have some time?