Add the web service implementation #4
Conversation
tkoscieln
force-pushed
the
use_fast_api
branch
2 times, most recently
from
e53d9ea to
497dcdb
Compare
|
@psss I've done some changes locally but am not able to push them here. Could you please check if I have the necessary rights? |
Sure, all should be set now. |
martinhoyer
force-pushed
the
use_fast_api
branch
from
8d997e8 to
9bdd574
Compare
|
Sorry for the mess with the workflow. Redis / KeyDB is driving me crazy. Trying to swithc to KeyDB due to Redis not having FLOSS license and KeyDB apparently being faster. |
martinhoyer
force-pushed
the
use_fast_api
branch
from
c16d93f to
0095668
Compare
|
fwiw, I'd love to debug it locally, but docker hub is rate limited :/ |
martinhoyer
force-pushed
the
use_fast_api
branch
from
585df24 to
a6cd7dc
Compare
src/api.py
Outdated
| plan_path: str = Query(None, alias="plan-path"), | ||
| out_format: str = Query("json", alias="format") | ||
| ): | ||
| # Parameter validations | ||
| if (test_url is None and test_name is not None) or (test_url is not None and test_name is None): | ||
| return "Invalid arguments!" |
There was a problem hiding this comment.
Invalid arguments should return 400-something status code. I think 400 is suitable one. See https://fastapi.tiangolo.com/tutorial/handling-errors/.
src/api.py
Outdated
| from fastapi.params import Query | ||
| from pydantic import BaseModel | ||
| from starlette.responses import HTMLResponse | ||
|
|
||
| from src import service |
There was a problem hiding this comment.
IMO it would be nicer if the python package would be named tmt_web or tmt.web (this one rather not, it could potentially collide with code in https://github.com/teemtee/tmt/). So the imports would look like e.g. from tmt_web import service.
There was a problem hiding this comment.
I don't think this is package name, but rather relative import from path, no? Should we add project metadata?
There was a problem hiding this comment.
Yeah, you're right, it's a relative import, +1 for project metadata.
There was a problem hiding this comment.
Relative imports must begin with a dot (eg. "from .src import service"), so this is a standard module/package import call. Python does add the current working directory to its list of module import lookup paths with the highest priority, so in the end it behaves the same way.
It's still a bad practice though and i'd suggest rearranging the project into what's the de-facto standard python project structure nowadays:
pyproject.toml
README.md
src/
tmt_web/
__init__.py
api.py
generators/
...
where a minimal working pyproject.toml could look something like this:
[build-system]
requires = ["setuptools>=64"]
build-backend = "setuptools.build_meta"
[project]
name = "tmt-web"
description = "TMT web app"
readme = "README.md"
requires-python = ">=3.10"
version = "0.0.1"
dependencies = [
"tmt~=1.35",
"fastapi~=0.112",
"httpx~=0.27",
"uvicorn~=0.30",
"celery[redis]~=5.4",
]
[tool.setuptools.packages.find]
where = ["src"]
https://packaging.python.org/en/latest/guides/writing-pyproject-toml/
There was a problem hiding this comment.
Thanks, I'm already working on it. +1 for standard src/tmt_web/
There was a problem hiding this comment.
btw are there any conventions about underscore vs dash vs .. ?
There was a problem hiding this comment.
project names should prefer dashes, while package/module/file names must use underscores
There was a problem hiding this comment.
Note that in PyPI the names are normalized, e.g. jaraco.context can be accessed as:
- https://pypi.org/project/jaraco.context/
- https://pypi.org/project/jaraco-context/
- https://pypi.org/project/JaRaCo_CoNtExT/
- etc.
and thus similarly there is no effect of what you choose in the dependencies as well. What is important though is making sure the sdist package is normalized to _, which I think only started happening in the recent 1-2 years. I am not sure about setuptools, but definitely going for hatchling build system does that for us.
But indeed usual convention for project.name is to use dashes -.
There was a problem hiding this comment.
@tkoscieln I kinda forgot about this. Let's change use a dash :)
martinhoyer
force-pushed
the
use_fast_api
branch
from
5e93e04 to
9d07a90
Compare
martinhoyer
force-pushed
the
use_fast_api
branch
3 times, most recently
from
688029a to
893b472
Compare
martinhoyer
force-pushed
the
use_fast_api
branch
from
980be55 to
568ffe9
Compare
Co-authored-by: Patrik Hagara <[email protected]>
* bump the dependencies to latest versions * fix the import of git_clone * no special reason for import alias * docstrings formatting and indentation problems * use f-strings instead * fix the order of imports * small improvements to readme * set execute permission to entrypoint.sh * small refactor of docstrings, unify quotation marks
* Add missing docstrings for return values * Move the settings to one place * Fix the typo in gitignore * Fix the RET505 lint * Provide service arguments as dictionary * Add a doctype to HTML template * Change page default title * Handle various types of responses when celery is disabled
There was a problem hiding this comment.
All major comments should be addressed and tests are green. Let's continue with further improvements in separate pull requests.
@tkoscieln, thanks a lot for implementing this!
This PR aims to introduce the complete service to the codebase. It includes a setup for deployment as well.