General improvements (pydanticv2, health checks, better testing, cleanup) #98
Conversation
tyler-romero
changed the title
app/api/routes/detectors.py
Outdated
There was a problem hiding this comment.
Anyone know of any reason why these were needed? I think these sort of passthroughs to the cloud should be handled by nginx.
app/api/remote.py
Outdated
pyproject.toml
Outdated
| framegrab = "^0.5.0" | ||
| groundlight = "^0.17.6" | ||
| groundlight = "<0.18.0" |
There was a problem hiding this comment.
Needed until the endpoint is updated to handle the new sdk response format for non-binary modes.
There was a problem hiding this comment.
See my comment on your other PR - we should really be pinning to a specific version here
app/api/routes/image_queries.py
Outdated
| logger.debug(f"Edge detector confidence is high enough to return. {detector_id=}") | ||
|
|
||
| result_type, results = _mode_to_result_type(detector_metadata.mode, results) | ||
| patience_time = patience_time or constants.DEFAULT_PATIENCE_TIME |
There was a problem hiding this comment.
This might create a small bug - this will go to the False case if patience_time is 0.0, right? (fixed in my incoming PR so no need to change here)
| @@ -41,19 +42,20 @@ def create_iqe( | |||
| return iq | |||
|
|
|||
|
|
|||
| def safe_call_api(api_method: Callable, **kwargs): | |||
| def safe_call_sdk(api_method: Callable, **kwargs): | |||
There was a problem hiding this comment.
Just flagging that this is what's creating issues with test_reset_retry (here) from the SDK tests.
tyler-romero
force-pushed
the
tyler/general-improvements-2
branch
from
a189299 to
4870100
Compare
| . test/setup_plain_test_env.sh && poetry run pytest -vs -k "not test_motdet" | ||
| . test/setup_plain_test_env.sh && poetry run pytest --cov=app --cov-report=lcov -vs -k "not test_motdet and not _live" | ||
|
|
||
| test-with-docker: install # Run tests that require a live edge-endpoint server and valid GL API token |
There was a problem hiding this comment.
Should we also have a test-all target that runs all the tests (Docker and non-Docker) in a single run.
| @@ -9,7 +10,10 @@ install-pre-commit: install ## Install pre-commit hooks. Requires .pre-commit-c | |||
| poetry run pre-commit install | |||
|
|
|||
| test: install ## Run unit tests in verbose mode | |||
| . test/setup_plain_test_env.sh && poetry run pytest -vs -k "not test_motdet" | |||
| . test/setup_plain_test_env.sh && poetry run pytest --cov=app --cov-report=lcov -vs -k "not test_motdet and not _live" | |||
There was a problem hiding this comment.
There seems to be no default situation that includes the motion detection test. Is this intentional?
There was a problem hiding this comment.
You might think that, but we run them in their own separate job... Probably worth consolidating that too.
Adds to previous code to fully support new mode structure. Multiclass is not supported for the time being but will be easy to add. --------- Co-authored-by: Auto-format Bot <autoformatbot@groundlight.ai> Co-authored-by: Tyler Romero <tyler@groundlight.ai>
Clean up the codebase and make some general improvements:
remote.py, the detectors endpoints)