pytest-api test: fix ping server step

[odulcy-mindee]

This PR aims two solve problems:

• to fix this error on CI : https://github.com/mindee/doctr/runs/7517599231?check_suite_focus=true
  curl command in CI is run but the API is not fully ready so the Ping server step often failed. There is an option --retry-all-errors since curl 7.71.0 but there is only curl 7.68.0 on ubuntu-latest (which is ubuntu 20.04) and that's not that simple to update curl to this version.
  I replaced curl by wget with option --spider.
  From man page:

--spider
When invoked with this option, Wget will behave as a Web spider, which means that it will not download the pages, just check that they are there.

• fix Doctr api doc error on docker #847. The problem comes from Tuples on fastapi, see Unexpected ValidationError with accessing /openapi.json when some model uses typing.Tuple tiangolo/fastapi#4168, openapi.json throws error when generated with nested models tiangolo/fastapi#3782 and FastAPI 0.68.0 Breaks Usage of tuple in Input / Output compared to 0.67.0 tiangolo/fastapi#3665 (comment). Updating fastapi version should fix the problem.

[charlesmindee]

LGTM thanks!

[odulcy-mindee]

Work on Ping server step also done here: #985

[felixdittrich92]

@frgfm

[felixdittrich92]

Hi @odulcy-mindee I think both PR's will not solve the under issue mentioned by users it is only a fix for CI 😅

[odulcy-mindee]

@felixdittrich92 Yeah, I just discovered that haha

[odulcy-mindee]

A deprecated decorator from pytest_asyncio induced failures on this step: https://github.com/mindee/doctr/runs/7522290771?check_suite_focus=true.

There has been a new release on pytest_asyncio. It requires to update the decorators. See a similar problem here pytest-dev/pytest-asyncio#390.

[codecov]

Codecov Report

Merging #997 (6385990) into main (23d1a1e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #997   +/-   ##
=======================================
  Coverage   94.85%   94.85%           
=======================================
  Files         134      134           
  Lines        5558     5558           
=======================================
  Hits         5272     5272           
  Misses        286      286           

Flag	Coverage Δ
unittests	94.85% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
doctr/transforms/functional/base.py	95.65% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us.

[felixdittrich92]

@odulcy-mindee I think to solve this issue we should add tests for different envs (Linux, Windows, Mac) wdyt ? I have had some trouble to solve it in fact that i was not able to reproduce the issue :)

[frgfm]

Hey everyone 👋

Here are my humble thoughts about this:

• for multi-OS testing, I think that would clutter the CI. This type of API is usually run on a Docker or similar isolated environment, so it makes sense that this is run a Linux-based docker
• about the underlying steps, I've started checking the proper version specifier range for all our dependencies on the API, the underlying issue should be fixed soon :)
• regarding the arg deprecation of pytest_asyncio, I've also experienced this on other projects, I'll fix this 👍

No need to close this PR, there are a few things we'll need 👌

[odulcy-mindee]

Ok, I was able to reproduce #847 on my Linux.

As @frgfm said, I think that it's still fine to test only on a Linux-based container. I'll continue to investigate the dependencies issues.

[felixdittrich92]

Nice 👍 @frgfm has also something dependency related open in #957

[odulcy-mindee]

@felixdittrich92 @frgfm The problem is coming from Tuples on fastapi, see tiangolo/fastapi#4168, tiangolo/fastapi#3782 and tiangolo/fastapi#3665 (comment).

I tested fastapi>=0.73.0 and it worked 🥳

[felixdittrich92]

Thanks a lot looks good for me :)
@frgfm wdyt?

[odulcy-mindee]

@felixdittrich92 Yes, you're right, I'll add a small note in requirements.txt

[felixdittrich92]

Thanks :)

[frgfm]

Thanks for the fix!
I'll check whether we can afford a looser version specifier (constraining pydantic in this case)

Something I don't get though:

• since the specifier used to be >=0.65.2, when the docker is built, it was taking the highest version available (I think there is no indirect constraints here), which is 0.79 (0.78 up until recently)
• so the only reason I can see that this change fixed the bug, is because the docker couldn't reuse the cache.

Is there something I missed? According to the threads, the faulty fastapi version range is within (0.66 - 0.73).

thanks!

[frgfm]

Could you remind me the reason of this change? (not familiar with the spider option, the "tries" flag seems explicit)

[felixdittrich92]

spider does only an 'exist' check and avoid downloading anything

[odulcy-mindee]

@frgfm Also, curl has an option --retry-all-errors to retry in any case but this option is only available in latest version of curl which can't be (easily) installed on Ubuntu 20.04. So it may still fail for various reasons that we can't catch. However, wget comes out of the box with a "retry on all errors".

[odulcy-mindee]

@frgfm Concerning your comment on version, it's a good remark. I didn't changed anything on my side and it didn't install the latest version of fastapi 🤔