From 0c50e58bc0b45d547278ed5ecce718a0bfe45d73 Mon Sep 17 00:00:00 2001 From: Alvaro Lopez Garcia Date: Tue, 25 Apr 2023 11:29:36 +0200 Subject: [PATCH 01/11] Include tox testing --- mypy.ini | 0 tox.ini | 140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+) create mode 100644 mypy.ini create mode 100644 tox.ini diff --git a/mypy.ini b/mypy.ini new file mode 100644 index 0000000..e69de29 diff --git a/tox.ini b/tox.ini new file mode 100644 index 0000000..ee714c3 --- /dev/null +++ b/tox.ini @@ -0,0 +1,140 @@ +[tox] +min_version = 4.3.3 +envlist = + py3{7, 8, 9, 10} + flake8 + black + bandit + mypy + pip-missing-reqs + pypi +skipsdist = True + +[gh-actions] +python = + 3.7: py37 + 3.8: py38 + 3.9: py39 + 3.10: py310, flake8, black, bandit, pip-missing-reqs, pypi + +[base] +python = python3.10 +skip_install = true +package = ai4papi + +[pytest] +addopts = -p no:warnings + +[testenv] +usedevelop = True +basepython = python3 +deps = + pytest>=7.2.1,<7.3 + pytest-cov>=4.0.0,<4.1 + -r{toxinidir}/requirements.txt +allowlist_externals = + find + rm + mkdir +setenv = + VIRTUAL_ENV={envdir} + LC_ALL=en_US.utf-8 + OS_STDOUT_CAPTURE=1 + OS_STDERR_CAPTURE=1 + OS_TEST_TIMEOUT=160 +commands = + find . -type f -name "*.pyc" -delete + pytest {posargs} + +[testenv:py37] +basepython = python3.7 + +[testenv:py38] +basepython = python3.8 + +[testenv:py39] +basepython = python3.9 + +[testenv:py310] +basepython = python3.10 + +[flake8] +# Black default line length is 88 +max-line-length = 88 +show-source = True +builtins = _ +exclude = + .venv + .git + .tox + dist + doc + *lib/python* + *egg + build + +[testenv:flake8] +basepython = {[base]python} +skip_install = {[base]skip_install} +deps = + flake8>=4.0,<4.1 + flake8-bugbear>=22.3,<22.4 + flake8-docstrings>=1.6,<1.7 + flake8-typing-imports>=1.12,<1.13 + flake8-colors>=0.1,<0.2 + pep8-naming>=0.12,<0.13 + pydocstyle>=6.1,<6.2 +commands = + flake8 + +[testenv:black] +basepython = {[base]python} +skip_install = {[base]skip_install} +deps = + black>=22.3,<22.4 +commands = black --check --diff {[base]package} + +[testenv:bandit] +basepython = {[base]python} +skip_install = {[base]skip_install} +deps = bandit +commands = bandit -r {[base]package} -x tests -s B110,B410 + +[testenv:bandit-report] +basepython = {[base]python} +deps = {[testenv:bandit]deps} +skip_install = {[base]skip_install} +commands = + - mkdir /tmp/bandit + - bandit -r {[base]package} -x tests -s B110,B410 -f html -o /tmp/bandit/index.html + +[testenv:pypi] +basepython = {[base]python} +skip_install = {[base]skip_install} +deps = docutils +commands = python3 setup.py check -r -s -m + +[testenv:genconfig] +basepython = {[base]python} +commands = oslo-config-generator --config-file=etc/{[base]package}-config-generator.conf + +[testenv:docs] +basepython = {[base]python} +deps = + -r {toxinidir}/doc/requirements.txt +commands = + rm -rf doc/build + python setup.py build_sphinx + +[testenv:mypy] +description = Static type checks +basepython = {[base]python} +deps = + mypy +commands = + mypy --config-file mypy.ini -p {[base]package} + +[testenv:pip-missing-reqs] +basepython = {[base]python} +deps = pip_check_reqs +commands=pip-missing-reqs -d --ignore-file={[base]package}/tests/* {[base]package} From 7a4dfcc34f9485febe117a1c673d8213c8d8c747 Mon Sep 17 00:00:00 2001 From: Alvaro Lopez Garcia Date: Thu, 27 Apr 2023 13:10:27 +0200 Subject: [PATCH 02/11] Only test our package --- tox.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tox.ini b/tox.ini index ee714c3..482b1a6 100644 --- a/tox.ini +++ b/tox.ini @@ -85,7 +85,7 @@ deps = pep8-naming>=0.12,<0.13 pydocstyle>=6.1,<6.2 commands = - flake8 + flake8 {[base]package} [testenv:black] basepython = {[base]python} From 113b953579c1335b1ae29912bd5d7aa05736f15e Mon Sep 17 00:00:00 2001 From: Alvaro Lopez Garcia Date: Thu, 27 Apr 2023 13:18:36 +0200 Subject: [PATCH 03/11] Add mypy configuration --- mypy.ini | 2 ++ 1 file changed, 2 insertions(+) diff --git a/mypy.ini b/mypy.ini index e69de29..976ba02 100644 --- a/mypy.ini +++ b/mypy.ini @@ -0,0 +1,2 @@ +[mypy] +ignore_missing_imports = True From d64e004809a6653443ae521760fbb3c992181c60 Mon Sep 17 00:00:00 2001 From: Alvaro Lopez Garcia Date: Thu, 27 Apr 2023 11:33:23 +0200 Subject: [PATCH 04/11] Add GH action for testing --- .github/workflows/ci.yml | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 .github/workflows/ci.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..d7d0324 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,28 @@ +name: Python testing + +on: + - push + - pull_request + +jobs: + test: + runs-on: ubuntu-latest + strategy: + matrix: + python-version: ["3.7", "3.8", "3.9", "3.10"] + + steps: + - uses: actions/checkout@v3 + + - name: Set up Python ${{ matrix.python-version }} + uses: actions/setup-python@v4 + with: + python-version: ${{ matrix.python-version }} + + - name: Install dependencies + run: | + python -m pip install --upgrade pip + pip install tox tox-gh-actions + + - name: Test with tox + run: tox From ad54329048a4b7484c78d23bbfbffe452b94ebe7 Mon Sep 17 00:00:00 2001 From: Alvaro Lopez Garcia Date: Thu, 27 Apr 2023 13:10:45 +0200 Subject: [PATCH 05/11] Make flake8 happy --- ai4papi/__init__.py | 2 + ai4papi/auth.py | 58 +++++++++---------- ai4papi/cli.py | 14 ++--- ai4papi/conf.py | 5 +- ai4papi/main.py | 16 +++--- ai4papi/routers/__init__.py | 1 + ai4papi/routers/v1/__init__.py | 2 + ai4papi/routers/v1/deployments.py | 95 +++++++++++++++---------------- ai4papi/routers/v1/info.py | 27 ++++----- ai4papi/routers/v1/modules.py | 84 ++++++++++++--------------- ai4papi/version.py | 2 + 11 files changed, 148 insertions(+), 158 deletions(-) diff --git a/ai4papi/__init__.py b/ai4papi/__init__.py index 7c4ad70..618a54c 100644 --- a/ai4papi/__init__.py +++ b/ai4papi/__init__.py @@ -14,6 +14,8 @@ # License for the specific language governing permissions and limitations # under the License. +"""AI4EOSC Platform API.""" + from . import version __version__ = version.release_string diff --git a/ai4papi/auth.py b/ai4papi/auth.py index c4399d5..90b46af 100644 --- a/ai4papi/auth.py +++ b/ai4papi/auth.py @@ -1,25 +1,24 @@ -""" -Authentication for private methods of the API (mainly managing deployments) - -Implementation notes: -==================== -Authentication is implemented using `get_user_infos_from_access_token` instead -of `get_user_infos_from_request` (as done in the FastAPI example in the flaat docs). -There are two advantages of this: -* the main one is that it would enable us enable to take advantage of Swagger's builtin - Authentication and Authorization [1] in the Swagger interface (generated by FastAPI). - This is not possible using the `Request` object, as data from `Request` cannot be validated and - documented by OpenAPI [2]. - - [1] https://swagger.io/docs/specification/authentication/ - [2] https://fastapi.tiangolo.com/advanced/using-request-directly/?h=request#details-about-the-request-object - -* the decorator `flaat.is_authenticated()` around each function is no longer needed, - as authentication is checked automatically by `authorization=Depends(security)` without needing extra code. - -The curl calls still remain the same, but now in the http://localhost/docs you will see an authorize - button where you can copy paste your token. So you will be able to access authenticated methods from the interface. -""" +"""Authentication for private methods of the API (mainly managing deployments).""" + +# Implementation notes: +# Authentication is implemented using `get_user_infos_from_access_token` instead +# of `get_user_infos_from_request` (as done in the FastAPI example in the flaat docs). +# There are two advantages of this: +# * the main one is that it would enable us enable to take advantage of Swagger's +# builtin Authentication and Authorization [1] in the Swagger interface (generated by +# FastAPI). This is not possible using the `Request` object, as data from `Request` +# cannot be validated and documented by OpenAPI [2]. +# +# [1] https://swagger.io/docs/specification/authentication/ +# [2] https://fastapi.tiangolo.com/advanced/using-request-directly +# +# * the decorator `flaat.is_authenticated()` around each function is no longer needed, +# as authentication is checked automatically by `authorization=Depends(security)` +# without needing extra code. +# +# The curl calls still remain the same, but now in the http://localhost/docs you will +# see an authorize button where you can copy paste your token. So you will be able to +# access authenticated methods from the interface. import re @@ -35,14 +34,14 @@ def get_user_info(token): - + """Return user information from a token.""" try: user_infos = flaat.get_user_infos_from_access_token(token) except Exception as e: raise HTTPException( status_code=403, detail=str(e), - ) + ) # Check output if user_infos is None: @@ -50,13 +49,13 @@ def get_user_info(token): status_code=403, detail="Invalid token", ) - + # Check scopes if user_infos.get('eduperson_entitlement') is None: raise HTTPException( status_code=403, - detail="Check you enabled the `eduperson_entitlement` scope for your token.", - ) + detail="Check you enabled the `eduperson_entitlement` scope for the token.", + ) # Parse Virtual Organizations manually from URNs # If more complexity is need in the future, check https://github.com/oarepo/urnparse @@ -76,7 +75,8 @@ def get_user_info(token): if not vos: raise HTTPException( status_code=403, - detail=f"You should belong to at least one of the Virtual Organizations supported by the project: {vos}.", + detail="You should belong to at least one of the Virtual Organizations " + f"supported by the project: {vos}.", ) # Generate user info dict @@ -84,7 +84,7 @@ def get_user_info(token): 'id': user_infos.get('sub'), # subject, user-ID 'issuer': user_infos.get('iss'), # URL of the access token issuer 'name': user_infos.get('name'), - 'vo': vos, + 'vo': vos, } return out diff --git a/ai4papi/cli.py b/ai4papi/cli.py index fe475c6..b382078 100644 --- a/ai4papi/cli.py +++ b/ai4papi/cli.py @@ -1,12 +1,9 @@ -""" -Command Line Interface to the API +"""Command Line Interface to the API.""" -Implementation notes: -==================== -Typer is not implement directly as a decorator around the run() function in main.py -because it gave errors while trying to run `uvicorn main:app --reload`. -Just have it in a separate file, clean and easy. -""" +# Implementation notes: +# Typer is not implement directly as a decorator around the run() function in main.py +# because it gave errors while trying to run `uvicorn main:app --reload`. Just have it +# in a separate file, clean and easy. import typer @@ -14,6 +11,7 @@ def run(): + """Run the API.""" typer.run(main.run) diff --git a/ai4papi/conf.py b/ai4papi/conf.py index d3cf6bc..a2afc88 100644 --- a/ai4papi/conf.py +++ b/ai4papi/conf.py @@ -1,6 +1,4 @@ -""" -Manage configurations of the API. -""" +"""Manage configurations of the API.""" from pathlib import Path @@ -33,4 +31,3 @@ assert 'value' in v.keys(), f"Parameter {k} needs to have a value." USER_CONF_VALUES[group_name][k] = v['value'] - diff --git a/ai4papi/main.py b/ai4papi/main.py index 518f1f8..d6fbe1a 100644 --- a/ai4papi/main.py +++ b/ai4papi/main.py @@ -1,6 +1,4 @@ -""" -Create an app with FastAPI -""" +"""AI4EOSC Plaform API (AI4PAPI) with FastAPI.""" import fastapi from fastapi.security import HTTPBearer @@ -38,6 +36,7 @@ def root( request: fastapi.Request, ): + """Get version and documentation endpoints.""" root = str(request.url_for("root")) versions = [v1.get_version(request)] @@ -65,11 +64,12 @@ def root( def run( - host:str = "0.0.0.0", - port:int = 8080, - ssl_keyfile:str = None, - ssl_certfile:str = None, - ): + host: str = "0.0.0.0", + port: int = 8080, + ssl_keyfile: str = None, + ssl_certfile: str = None, +): + """Run the API using uvicorn.""" uvicorn.run( app, host=host, diff --git a/ai4papi/routers/__init__.py b/ai4papi/routers/__init__.py index e69de29..2e8904e 100644 --- a/ai4papi/routers/__init__.py +++ b/ai4papi/routers/__init__.py @@ -0,0 +1 @@ +"""Route definition and management.""" diff --git a/ai4papi/routers/v1/__init__.py b/ai4papi/routers/v1/__init__.py index 2bcbcc8..43a8f33 100644 --- a/ai4papi/routers/v1/__init__.py +++ b/ai4papi/routers/v1/__init__.py @@ -1,3 +1,4 @@ +"""V1 routes.""" import fastapi @@ -16,6 +17,7 @@ tags=["API", "version"], ) def get_version(request: fastapi.Request): + """Get V1 version information.""" root = str(request.url_for("get_version")) # root = "/" version = { diff --git a/ai4papi/routers/v1/deployments.py b/ai4papi/routers/v1/deployments.py index aa76f9c..cb708a7 100644 --- a/ai4papi/routers/v1/deployments.py +++ b/ai4papi/routers/v1/deployments.py @@ -1,12 +1,8 @@ -""" -Manage deployments with Nomad. -This is the AI4EOSC Training API. +"""API routest that manage deployments with Nomad.""" -Notes: -===== -* Terminology warning: what we call a "deployment" (as in `create_deployment`) - is a Nomad "job" (not a Nomad "deployment"!) -""" +# Notes: +# * Terminology warning: what we call a "deployment" (as in `create_deployment`) is a +# Nomad "job" (not a Nomad "deployment"!) from copy import deepcopy from datetime import datetime @@ -34,15 +30,11 @@ @router.get("/") -def get_deployments( - authorization=Depends(security), - ): - """ - Returns a list of all deployments belonging to a user. - """ +def get_deployments(authorization=Depends(security)): + """Return a list of all deployments belonging to a user.""" # Retrieve authenticated user info auth_info = get_user_info(token=authorization.credentials) - owner, provider = auth_info['id'], auth_info['vo'] + owner, provider = auth_info['id'], auth_info['vo'] # noqa(F841) # Filter jobs jobs = Nomad.jobs.get_jobs() # job summaries @@ -76,12 +68,10 @@ def get_deployments( @router.get("/{deployment_uuid}") -def get_deployment( - deployment_uuid: str, - authorization=Depends(security), - ): +def get_deployment(deployment_uuid: str, authorization=Depends(security)): """ Retrieve the info of a specific deployment. + Format outputs to a Nomad-independent format to be used by the Dashboard Parameters: @@ -91,7 +81,7 @@ def get_deployment( """ # Retrieve authenticated user info auth_info = get_user_info(token=authorization.credentials) - owner, provider = auth_info['id'], auth_info['vo'] + owner, provider = auth_info['id'], auth_info['vo'] # noqa(F841) # Check the deployment exists try: @@ -100,14 +90,14 @@ def get_deployment( raise HTTPException( status_code=403, detail="No deployment exists with this uuid.", - ) + ) # Check job does belong to owner if j['Meta'] and owner != j['Meta'].get('owner', ''): raise HTTPException( status_code=403, detail="You are not the owner of that deployment.", - ) + ) # Create job info dict info = { @@ -133,15 +123,17 @@ def get_deployment( # Only fill (resources + endpoints) if the job is allocated allocs = Nomad.job.get_allocations(j['ID']) if allocs: - a = Nomad.allocation.get_allocation(allocs[0]['ID']) # only keep the first allocation per job + # only keep the first allocation per job + a = Nomad.allocation.get_allocation(allocs[0]['ID']) info['alloc_ID'] = a['ID'] res = a['AllocatedResources'] devices = res['Tasks']['usertask']['Devices'] + gpu_num = sum([1 for d in devices if d['Type'] == 'gpu']) if devices else 0 info['resources'] = { 'cpu_num': res['Tasks']['usertask']['Cpu']['CpuShares'], - 'gpu_num': sum([1 for d in devices if d['Type'] == 'gpu']) if devices else 0, + 'gpu_num': gpu_num, 'memoryMB': res['Tasks']['usertask']['Memory']['MemoryMB'], 'diskMB': res['Shared']['DiskMB'], } @@ -149,24 +141,24 @@ def get_deployment( public_ip = 'https://xxx.xxx.xxx.xxx' # todo: replace when ready ports = a['Resources']['Networks'][0]['DynamicPorts'] info['endpoints'] = {d['Label']: f"{public_ip}:{d['Value']}" for d in ports} - # todo: We need to connect internal IP (172.XXX) to external IP (193.XXX) (Traefik + Consul Connect) - # use service discovery to map internal ip to external IPs??? + # TODO: We need to connect internal IP (172.XXX) to external IP (193.XXX) + # (Traefik + Consul Connect) use service discovery to map internal ip to + # external IPs??? else: - # Something happened, job didn't deploy (eg. jobs needs port that's currently being used) + # Something happened, job didn't deploy (eg. jobs needs port that's currently + # being used) # We have to return `placement failures message`. evals = Nomad.job.get_evaluations(j['ID']) info['error_msg'] = f"{evals[0]['FailedTGAllocs']}" - # todo: improve this error message once we start seeing the different modes of failures in typical cases + # TODO: improve this error message once we start seeing the different modes of + # failures in typical cases return info @router.post("/") -def create_deployment( - conf: dict = {}, - authorization=Depends(security), - ): +def create_deployment(conf: dict = {}, authorization=Depends(security)): """ Submit a deployment to Nomad. @@ -179,7 +171,7 @@ def create_deployment( """ # Retrieve authenticated user info auth_info = get_user_info(token=authorization.credentials) - owner, provider = auth_info['id'], auth_info['vo'] + owner, provider = auth_info['id'], auth_info['vo'] # noqa(F841) # Update default dict with new values job_conf, user_conf = deepcopy(NOMAD_JOB_CONF), deepcopy(USER_CONF_VALUES) @@ -189,24 +181,29 @@ def create_deployment( if ( user_conf['general']['service'] == 'jupyterlab' and len(user_conf['general']['jupyter_password']) < 9 - ): + ): raise HTTPException( status_code=501, detail="JupyterLab password should have at least 9 characters." ) - # Assign unique job ID (if submitting job with existing ID, the existing job gets replaced) - job_conf['ID'] = f'userjob-{uuid.uuid1()}' # id is generated from (MAC address+timestamp) so it's unique + # Assign unique job ID (if submitting job with existing ID, the existing job gets + # replaced) + # id is generated from (MAC address+timestamp) so it's unique + job_conf['ID'] = f'userjob-{uuid.uuid1()}' # Add owner and extra information to the job metadata job_conf['Meta']['owner'] = owner - job_conf['Meta']['title'] = user_conf['general']['title'][:45] # keep only 45 first characters - job_conf['Meta']['description'] = user_conf['general']['desc'][:1000] # limit to 1K characters + # keep only 45 first characters + job_conf['Meta']['title'] = user_conf['general']['title'][:45] + # limit to 1K characters + job_conf['Meta']['description'] = user_conf['general']['desc'][:1000] # Replace user conf in Nomad job task = job_conf['TaskGroups'][0]['Tasks'][0] - task['Config']['image'] = f"{user_conf['general']['docker_image']}:{user_conf['general']['docker_tag']}" + img = f"{user_conf['general']['docker_image']}:{user_conf['geneal']['docker_tag']}" + task['Config']['image'] = img task['Config']['command'] = "deep-start" task['Config']['args'] = [f"--{user_conf['general']['service']}"] @@ -222,7 +219,9 @@ def create_deployment( if not user_conf['hardware']['gpu_type']: del task['Resources']['Devices'][0]['Affinities'] else: - task['Resources']['Devices'][0]['Affinities'][0]['RTarget'] = user_conf['hardware']['gpu_type'] + task['Resources']['Devices'][0]['Affinities'][0]['RTarget'] = ( + user_conf['hardware']['gpu_type'] + ) task['Env']['RCLONE_CONFIG_RSHARE_URL'] = user_conf['storage']['rclone_url'] task['Env']['RCLONE_CONFIG_RSHARE_VENDOR'] = user_conf['storage']['rclone_vendor'] @@ -233,7 +232,7 @@ def create_deployment( # Submit job try: - response = Nomad.jobs.register_job({'Job': job_conf}) + response = Nomad.jobs.register_job({'Job': job_conf}) # noqa(F841) return { 'status': 'success', 'job_id': job_conf['ID'], @@ -246,12 +245,10 @@ def create_deployment( @router.delete("/{deployment_uuid}") -def delete_deployment( - deployment_uuid: str, - authorization=Depends(security), - ): - """ - Delete a deployment. Users can only delete their own deployments. +def delete_deployment(deployment_uuid: str, authorization=Depends(security)): + """Delete a deployment. + + Users can only delete their own deployments. Parameters: * **deployment_uuid**: uuid of deployment to delete @@ -260,7 +257,7 @@ def delete_deployment( """ # Retrieve authenticated user info auth_info = get_user_info(token=authorization.credentials) - owner, provider = auth_info['id'], auth_info['vo'] + owner, provider = auth_info['id'], auth_info['vo'] # noqa(F841) # Check the deployment exists try: @@ -276,7 +273,7 @@ def delete_deployment( raise HTTPException( status_code=403, detail="You are not the owner of that deployment.", - ) + ) # Delete deployment Nomad.job.deregister_job(deployment_uuid) diff --git a/ai4papi/routers/v1/info.py b/ai4papi/routers/v1/info.py index afa3944..08e9b31 100644 --- a/ai4papi/routers/v1/info.py +++ b/ai4papi/routers/v1/info.py @@ -1,8 +1,7 @@ -""" -Misc routes. +"""Misc routes. -Methods returning the conf are authenticated in order to be -able to fill the `Virtual Organization` field. +Methods returning the conf are authenticated in order to be able to fill the `Virtual +Organization` field. """ from copy import deepcopy @@ -23,6 +22,8 @@ security = HTTPBearer() +base_mod_url = "https://registry.hub.docker.com/v2/repositories/deephdc/" + @router.get("/conf/{module_name}") def get_default_deployment_conf( @@ -30,13 +31,13 @@ def get_default_deployment_conf( authorization=Depends(security), ): """ - Returns the default configuration (dict) for creating a deployment - for a specific module. It is prefilled with the appropriate - docker image and the available docker tags. + Return the default configuration to creating a deployment for a module. + + It is prefilled with the appropriate docker image and the available docker tags. - We are not checking if module exists in the marketplace because - we are treating each route as independent. In the future, this can - be done as an API call to the other route. + We are not checking if module exists in the marketplace because we are treating each + route as independent. In the future, this can be done as an API call to the other + route. """ # Retrieve authenticated user info auth_info = get_user_info(token=authorization.credentials) @@ -53,16 +54,16 @@ def get_default_deployment_conf( conf["general"]["docker_image"]["value"] = f"deephdc/{module_name.lower()}" # Add available Docker tags - url = f"https://registry.hub.docker.com/v2/repositories/deephdc/{module_name.lower()}/tags" + url = f"{base_mod_url}/{module_name.lower()}/tags" try: r = requests.get(url) r.raise_for_status() r = r.json() - except Exception as e: + except Exception: raise HTTPException( status_code=400, detail="Could not retrieve Docker tags from {module_name}.", - ) + ) tags = [i["name"] for i in r["results"]] conf["general"]["docker_tag"]["options"] = tags diff --git a/ai4papi/routers/v1/modules.py b/ai4papi/routers/v1/modules.py index 67d29da..b295c44 100644 --- a/ai4papi/routers/v1/modules.py +++ b/ai4papi/routers/v1/modules.py @@ -1,26 +1,24 @@ -""" -Manage module metadata. -This is the AI4EOSC Exchange API. - -Implementation notes: -==================== -* We decide to initially implement this in the same repo/API as deployment management (Training API), -but on a different API route. -This is done to avoid duplicating code (eg. authentication) in the initial development phase. -We can always move this to another repo/API in the future if needed. - -* Output caching -We are in a transition time, where users still edit their modules metadata in their Github repos. -But reading metadata from Github and returning it isn't fast enough for a seamless experience (~0.3s / module). -This is especially critical for the Overview of the Dashboard where one retrieves metadata of all the modules (N * 0.3s). -We decide to cache the outputs of the functions for up to six hours to enable fast calls in the meantime. -This caching can be eventually removed when we move the metadata to the Exchange database. -Or the caching time be reduced if needed, though 6 hours seems to be a decent compromise, as metadata is not constantly changing. -""" +"""API routes that manage module metadata.""" + +# Implementation notes: +# ==================== +# * We decide to initially implement this in the same repo/API as deployment management +# (Training API), but on a different API route. This is done to avoid duplicating code +# (eg. authentication) in the initial development phase. We can always move this to +# another repo/API in the future if needed. +# +# * Output caching +# We are in a transition time, where users still edit their modules metadata in their +# Github repos. But reading metadata from Github and returning it isn't fast enough for +# a seamless experience (~0.3s / module). This is especially critical for the Overview +# of the Dashboard where one retrieves metadata of all the modules (N * 0.3s). We +# decide to cache the outputs of the functions for up to six hours to enable fast calls +# in the meantime. This caching can be eventually removed when we move the metadata to +# the Exchange database. Or the caching time be reduced if needed, though 6 hours seems +# to be a decent compromise, as metadata is not constantly changing. import configparser import json -import re from cachetools import cached, TTLCache from fastapi import APIRouter, HTTPException @@ -34,14 +32,14 @@ responses={404: {"description": "Not found"}}, ) +base_org_url = "https://raw.githubusercontent.com/deephdc/" + @router.get("/") @cached(cache=TTLCache(maxsize=1024, ttl=6*60*60)) def get_modules_list(): - """ - Retrieve a list of all modules. - """ - modules_url = "https://raw.githubusercontent.com/deephdc/deep-oc/master/MODULES.yml" + """Retrieve a list of all modules.""" + modules_url = f"{base_org_url}/deep-oc/master/MODULES.yml" r = requests.get(modules_url) catalog = yaml.safe_load(r.text) modules = [i['module'].split('/')[-1] for i in catalog] # remove github prefix @@ -51,9 +49,9 @@ def get_modules_list(): @router.get("/summary") @cached(cache=TTLCache(maxsize=1024, ttl=6*60*60)) def get_modules_summary(): - """ - Retrieve a list of all modules' basic metadata - (name, title, summary, keywords). + """Retrieve a list of modules with basic metadata. + + This will return a list of (name, title, summary, keywords). """ summary = [] keys = ['title', 'summary', 'keywords'] @@ -67,23 +65,18 @@ def get_modules_summary(): @router.get("/metadata/{module_name}") @cached(cache=TTLCache(maxsize=1024, ttl=6*60*60)) -def get_module_metadata( - module_name: str, - ): - """ - Get the module's full metadata. - """ - +def get_module_metadata(module_name: str): + """Get the module's full metadata.""" # Check the module is in the modules list modules = get_modules_list() if module_name not in modules: raise HTTPException( status_code=400, detail="Module {module_name} not in catalog: {modules}", - ) + ) # Find what is the default branch of the module - gitmodules_url = "https://raw.githubusercontent.com/deephdc/deep-oc/master/.gitmodules" + gitmodules_url = f"{base_org_url}/deep-oc/master/.gitmodules" r = requests.get(gitmodules_url) gitmodules_conf = configparser.ConfigParser() @@ -96,14 +89,14 @@ def get_module_metadata( branch = "master" # Retrieve metadata from that branch - metadata_url = f"https://raw.githubusercontent.com/deephdc/{module_name}/{branch}/metadata.json" + metadata_url = f"{base_org_url}/{module_name}/{branch}/metadata.json" r = requests.get(metadata_url) metadata = json.loads(r.text) # Format "description" field nicely for the Dashboards Markdown parser desc = [] - for l in metadata['description']: - desc.append(l) + for aux in metadata['description']: + desc.append(aux) metadata["description"] = "\n".join(desc) # single string @@ -111,12 +104,9 @@ def get_module_metadata( @router.put("/metadata/{module_name}") -def update_module_metadata( - module_name: str, - ): - """ - Update the module's metadata. - TODO: do this when we implement the AI4EOSC Exchange database - This function needs authentication, users should only be able to edit their own modules - """ +def update_module_metadata(module_name: str): + """Update the module's metadata.""" + # TODO: do this when we implement the AI4EOSC Exchange database + # This function needs authentication, users should only be able to edit their own + # modules raise HTTPException(status_code=501) diff --git a/ai4papi/version.py b/ai4papi/version.py index 98e784a..f27e904 100644 --- a/ai4papi/version.py +++ b/ai4papi/version.py @@ -14,6 +14,8 @@ # License for the specific language governing permissions and limitations # under the License. +"""AI4EOSC Platform API version information.""" + import pbr.version version_info = pbr.version.VersionInfo("ai4papi") From 281e6e2a08bda8de1db977f427d7b6279a1d66c3 Mon Sep 17 00:00:00 2001 From: Alvaro Lopez Garcia Date: Thu, 27 Apr 2023 13:12:47 +0200 Subject: [PATCH 06/11] Use a module-level variable for funtion-level definitions Instead of using a function-level definition for the dependency on the bearer tokens, lets move it outside, to a module-level variable. --- ai4papi/routers/v1/deployments.py | 10 +++++----- ai4papi/routers/v1/info.py | 7 ++----- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/ai4papi/routers/v1/deployments.py b/ai4papi/routers/v1/deployments.py index cb708a7..f015b7b 100644 --- a/ai4papi/routers/v1/deployments.py +++ b/ai4papi/routers/v1/deployments.py @@ -24,13 +24,13 @@ responses={404: {"description": "Not found"}}, ) -security = HTTPBearer() +security = Depends(HTTPBearer()) Nomad = nomad.Nomad() @router.get("/") -def get_deployments(authorization=Depends(security)): +def get_deployments(authorization=security): """Return a list of all deployments belonging to a user.""" # Retrieve authenticated user info auth_info = get_user_info(token=authorization.credentials) @@ -68,7 +68,7 @@ def get_deployments(authorization=Depends(security)): @router.get("/{deployment_uuid}") -def get_deployment(deployment_uuid: str, authorization=Depends(security)): +def get_deployment(deployment_uuid: str, authorization=security): """ Retrieve the info of a specific deployment. @@ -158,7 +158,7 @@ def get_deployment(deployment_uuid: str, authorization=Depends(security)): @router.post("/") -def create_deployment(conf: dict = {}, authorization=Depends(security)): +def create_deployment(conf: dict = {}, authorization=security): """ Submit a deployment to Nomad. @@ -245,7 +245,7 @@ def create_deployment(conf: dict = {}, authorization=Depends(security)): @router.delete("/{deployment_uuid}") -def delete_deployment(deployment_uuid: str, authorization=Depends(security)): +def delete_deployment(deployment_uuid: str, authorization=security): """Delete a deployment. Users can only delete their own deployments. diff --git a/ai4papi/routers/v1/info.py b/ai4papi/routers/v1/info.py index 08e9b31..c2a882b 100644 --- a/ai4papi/routers/v1/info.py +++ b/ai4papi/routers/v1/info.py @@ -20,16 +20,13 @@ responses={404: {"description": "Not found"}}, ) -security = HTTPBearer() +security = Depends(HTTPBearer()) base_mod_url = "https://registry.hub.docker.com/v2/repositories/deephdc/" @router.get("/conf/{module_name}") -def get_default_deployment_conf( - module_name: str, - authorization=Depends(security), -): +def get_default_deployment_conf(module_name: str, authorization=security): """ Return the default configuration to creating a deployment for a module. From 7448e88af1564df0f45dce9ad5e707d9696b790b Mon Sep 17 00:00:00 2001 From: Alvaro Lopez Garcia Date: Thu, 27 Apr 2023 13:17:11 +0200 Subject: [PATCH 07/11] Do not use mutable dict as argument default --- ai4papi/routers/v1/deployments.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ai4papi/routers/v1/deployments.py b/ai4papi/routers/v1/deployments.py index f015b7b..f162d6e 100644 --- a/ai4papi/routers/v1/deployments.py +++ b/ai4papi/routers/v1/deployments.py @@ -158,7 +158,7 @@ def get_deployment(deployment_uuid: str, authorization=security): @router.post("/") -def create_deployment(conf: dict = {}, authorization=security): +def create_deployment(conf: dict | None = None, authorization=security): """ Submit a deployment to Nomad. @@ -175,7 +175,8 @@ def create_deployment(conf: dict = {}, authorization=security): # Update default dict with new values job_conf, user_conf = deepcopy(NOMAD_JOB_CONF), deepcopy(USER_CONF_VALUES) - user_conf.update(conf) + if conf: + user_conf.update(conf) # Enforce JupyterLab password minimum number of characters if ( From 4e2bd7ff46877814f5a04aecd3ff8b921bf654f4 Mon Sep 17 00:00:00 2001 From: Alvaro Lopez Garcia Date: Thu, 27 Apr 2023 13:17:55 +0200 Subject: [PATCH 08/11] Make code black --- ai4papi/auth.py | 26 +++-- ai4papi/conf.py | 14 +-- ai4papi/routers/v1/deployments.py | 158 +++++++++++++++--------------- ai4papi/routers/v1/info.py | 2 +- ai4papi/routers/v1/modules.py | 14 +-- 5 files changed, 105 insertions(+), 109 deletions(-) diff --git a/ai4papi/auth.py b/ai4papi/auth.py index 90b46af..5781adb 100644 --- a/ai4papi/auth.py +++ b/ai4papi/auth.py @@ -48,10 +48,10 @@ def get_user_info(token): raise HTTPException( status_code=403, detail="Invalid token", - ) + ) # Check scopes - if user_infos.get('eduperson_entitlement') is None: + if user_infos.get("eduperson_entitlement") is None: raise HTTPException( status_code=403, detail="Check you enabled the `eduperson_entitlement` scope for the token.", @@ -60,15 +60,11 @@ def get_user_info(token): # Parse Virtual Organizations manually from URNs # If more complexity is need in the future, check https://github.com/oarepo/urnparse vos = [] - for i in user_infos.get('eduperson_entitlement'): - vos.append( - re.search(r"group:(.+?):", i).group(1) - ) + for i in user_infos.get("eduperson_entitlement"): + vos.append(re.search(r"group:(.+?):", i).group(1)) # Filter VOs to keep only the ones relevant to us - vos = set(vos).intersection( - set(MAIN_CONF['auth']['VO']) - ) + vos = set(vos).intersection(set(MAIN_CONF["auth"]["VO"])) vos = sorted(vos) # Check if VOs is empty after filtering @@ -76,15 +72,15 @@ def get_user_info(token): raise HTTPException( status_code=403, detail="You should belong to at least one of the Virtual Organizations " - f"supported by the project: {vos}.", - ) + f"supported by the project: {vos}.", + ) # Generate user info dict out = { - 'id': user_infos.get('sub'), # subject, user-ID - 'issuer': user_infos.get('iss'), # URL of the access token issuer - 'name': user_infos.get('name'), - 'vo': vos, + "id": user_infos.get("sub"), # subject, user-ID + "issuer": user_infos.get("iss"), # URL of the access token issuer + "name": user_infos.get("name"), + "vo": vos, } return out diff --git a/ai4papi/conf.py b/ai4papi/conf.py index a2afc88..374dd6a 100644 --- a/ai4papi/conf.py +++ b/ai4papi/conf.py @@ -8,26 +8,26 @@ Nomad = nomad.Nomad() -CONF_DIR = Path(__file__).parents[1].absolute() / 'etc' +CONF_DIR = Path(__file__).parents[1].absolute() / "etc" # Load main API configuration -with open(CONF_DIR / 'main_conf.yaml', 'r') as f: +with open(CONF_DIR / "main_conf.yaml", "r") as f: MAIN_CONF = yaml.safe_load(f) # Load default Nomad job configuration -with open(CONF_DIR / 'job.nomad', 'r') as f: +with open(CONF_DIR / "job.nomad", "r") as f: job_raw_nomad = f.read() NOMAD_JOB_CONF = Nomad.jobs.parse(job_raw_nomad) # Load user customizable parameters -with open(CONF_DIR / 'userconf.yaml', 'r') as f: +with open(CONF_DIR / "userconf.yaml", "r") as f: USER_CONF = yaml.safe_load(f) USER_CONF_VALUES = {} for group_name, params in USER_CONF.items(): USER_CONF_VALUES[group_name] = {} for k, v in params.items(): - assert 'name' in v.keys(), f"Parameter {k} needs to have a name." - assert 'value' in v.keys(), f"Parameter {k} needs to have a value." + assert "name" in v.keys(), f"Parameter {k} needs to have a name." + assert "value" in v.keys(), f"Parameter {k} needs to have a value." - USER_CONF_VALUES[group_name][k] = v['value'] + USER_CONF_VALUES[group_name][k] = v["value"] diff --git a/ai4papi/routers/v1/deployments.py b/ai4papi/routers/v1/deployments.py index f162d6e..2923082 100644 --- a/ai4papi/routers/v1/deployments.py +++ b/ai4papi/routers/v1/deployments.py @@ -34,21 +34,21 @@ def get_deployments(authorization=security): """Return a list of all deployments belonging to a user.""" # Retrieve authenticated user info auth_info = get_user_info(token=authorization.credentials) - owner, provider = auth_info['id'], auth_info['vo'] # noqa(F841) + owner, provider = auth_info["id"], auth_info["vo"] # noqa(F841) # Filter jobs jobs = Nomad.jobs.get_jobs() # job summaries njobs = [] for j in jobs: # Skip deleted jobs - if j['Status'] == 'dead': + if j["Status"] == "dead": continue # Get full job description - j = Nomad.job.get_job(j['ID']) + j = Nomad.job.get_job(j["ID"]) # Remove jobs not belonging to owner - if j['Meta'] and (owner == j['Meta'].get('owner', '')): + if j["Meta"] and (owner == j["Meta"].get("owner", "")): njobs.append(j) # Retrieve info for jobs @@ -56,7 +56,7 @@ def get_deployments(authorization=security): for j in njobs: job_info = get_deployment( - deployment_uuid=j['ID'], + deployment_uuid=j["ID"], authorization=SimpleNamespace( credentials=authorization.credentials # token ), @@ -81,7 +81,7 @@ def get_deployment(deployment_uuid: str, authorization=security): """ # Retrieve authenticated user info auth_info = get_user_info(token=authorization.credentials) - owner, provider = auth_info['id'], auth_info['vo'] # noqa(F841) + owner, provider = auth_info["id"], auth_info["vo"] # noqa(F841) # Check the deployment exists try: @@ -93,7 +93,7 @@ def get_deployment(deployment_uuid: str, authorization=security): ) # Check job does belong to owner - if j['Meta'] and owner != j['Meta'].get('owner', ''): + if j["Meta"] and owner != j["Meta"].get("owner", ""): raise HTTPException( status_code=403, detail="You are not the owner of that deployment.", @@ -101,46 +101,46 @@ def get_deployment(deployment_uuid: str, authorization=security): # Create job info dict info = { - 'job_ID': j['ID'], - 'status': j['Status'], - 'owner': j['Meta']['owner'], - 'title': j['Meta']['title'], - 'description': j['Meta']['description'], - 'docker_image': None, - 'submit_time': datetime.fromtimestamp( - j['SubmitTime'] // 1000000000 - ).strftime('%Y-%m-%d %H:%M:%S'), # nanoseconds to timestamp - 'resources': {}, - 'endpoints': {}, - 'alloc_ID': None, + "job_ID": j["ID"], + "status": j["Status"], + "owner": j["Meta"]["owner"], + "title": j["Meta"]["title"], + "description": j["Meta"]["description"], + "docker_image": None, + "submit_time": datetime.fromtimestamp(j["SubmitTime"] // 1000000000).strftime( + "%Y-%m-%d %H:%M:%S" + ), # nanoseconds to timestamp + "resources": {}, + "endpoints": {}, + "alloc_ID": None, } # Retrieve Docker image - for t in j['TaskGroups'][0]['Tasks']: - if t['Name'] == 'usertask': - info['docker_image'] = t['Config']['image'] + for t in j["TaskGroups"][0]["Tasks"]: + if t["Name"] == "usertask": + info["docker_image"] = t["Config"]["image"] # Only fill (resources + endpoints) if the job is allocated - allocs = Nomad.job.get_allocations(j['ID']) + allocs = Nomad.job.get_allocations(j["ID"]) if allocs: # only keep the first allocation per job - a = Nomad.allocation.get_allocation(allocs[0]['ID']) - - info['alloc_ID'] = a['ID'] - - res = a['AllocatedResources'] - devices = res['Tasks']['usertask']['Devices'] - gpu_num = sum([1 for d in devices if d['Type'] == 'gpu']) if devices else 0 - info['resources'] = { - 'cpu_num': res['Tasks']['usertask']['Cpu']['CpuShares'], - 'gpu_num': gpu_num, - 'memoryMB': res['Tasks']['usertask']['Memory']['MemoryMB'], - 'diskMB': res['Shared']['DiskMB'], + a = Nomad.allocation.get_allocation(allocs[0]["ID"]) + + info["alloc_ID"] = a["ID"] + + res = a["AllocatedResources"] + devices = res["Tasks"]["usertask"]["Devices"] + gpu_num = sum([1 for d in devices if d["Type"] == "gpu"]) if devices else 0 + info["resources"] = { + "cpu_num": res["Tasks"]["usertask"]["Cpu"]["CpuShares"], + "gpu_num": gpu_num, + "memoryMB": res["Tasks"]["usertask"]["Memory"]["MemoryMB"], + "diskMB": res["Shared"]["DiskMB"], } - public_ip = 'https://xxx.xxx.xxx.xxx' # todo: replace when ready - ports = a['Resources']['Networks'][0]['DynamicPorts'] - info['endpoints'] = {d['Label']: f"{public_ip}:{d['Value']}" for d in ports} + public_ip = "https://xxx.xxx.xxx.xxx" # todo: replace when ready + ports = a["Resources"]["Networks"][0]["DynamicPorts"] + info["endpoints"] = {d["Label"]: f"{public_ip}:{d['Value']}" for d in ports} # TODO: We need to connect internal IP (172.XXX) to external IP (193.XXX) # (Traefik + Consul Connect) use service discovery to map internal ip to # external IPs??? @@ -149,8 +149,8 @@ def get_deployment(deployment_uuid: str, authorization=security): # Something happened, job didn't deploy (eg. jobs needs port that's currently # being used) # We have to return `placement failures message`. - evals = Nomad.job.get_evaluations(j['ID']) - info['error_msg'] = f"{evals[0]['FailedTGAllocs']}" + evals = Nomad.job.get_evaluations(j["ID"]) + info["error_msg"] = f"{evals[0]['FailedTGAllocs']}" # TODO: improve this error message once we start seeing the different modes of # failures in typical cases @@ -171,7 +171,7 @@ def create_deployment(conf: dict | None = None, authorization=security): """ # Retrieve authenticated user info auth_info = get_user_info(token=authorization.credentials) - owner, provider = auth_info['id'], auth_info['vo'] # noqa(F841) + owner, provider = auth_info["id"], auth_info["vo"] # noqa(F841) # Update default dict with new values job_conf, user_conf = deepcopy(NOMAD_JOB_CONF), deepcopy(USER_CONF_VALUES) @@ -180,68 +180,68 @@ def create_deployment(conf: dict | None = None, authorization=security): # Enforce JupyterLab password minimum number of characters if ( - user_conf['general']['service'] == 'jupyterlab' and - len(user_conf['general']['jupyter_password']) < 9 + user_conf["general"]["service"] == "jupyterlab" + and len(user_conf["general"]["jupyter_password"]) < 9 ): raise HTTPException( status_code=501, - detail="JupyterLab password should have at least 9 characters." - ) + detail="JupyterLab password should have at least 9 characters.", + ) # Assign unique job ID (if submitting job with existing ID, the existing job gets # replaced) # id is generated from (MAC address+timestamp) so it's unique - job_conf['ID'] = f'userjob-{uuid.uuid1()}' + job_conf["ID"] = f"userjob-{uuid.uuid1()}" # Add owner and extra information to the job metadata - job_conf['Meta']['owner'] = owner + job_conf["Meta"]["owner"] = owner # keep only 45 first characters - job_conf['Meta']['title'] = user_conf['general']['title'][:45] + job_conf["Meta"]["title"] = user_conf["general"]["title"][:45] # limit to 1K characters - job_conf['Meta']['description'] = user_conf['general']['desc'][:1000] + job_conf["Meta"]["description"] = user_conf["general"]["desc"][:1000] # Replace user conf in Nomad job - task = job_conf['TaskGroups'][0]['Tasks'][0] + task = job_conf["TaskGroups"][0]["Tasks"][0] img = f"{user_conf['general']['docker_image']}:{user_conf['geneal']['docker_tag']}" - task['Config']['image'] = img - task['Config']['command'] = "deep-start" - task['Config']['args'] = [f"--{user_conf['general']['service']}"] + task["Config"]["image"] = img + task["Config"]["command"] = "deep-start" + task["Config"]["args"] = [f"--{user_conf['general']['service']}"] # TODO: add `docker_privileged` arg if we still need it - task['Resources']['CPU'] = user_conf['hardware']['cpu_num'] - task['Resources']['MemoryMB'] = user_conf['hardware']['ram'] - task['Resources']['DiskMB'] = user_conf['hardware']['disk'] - if user_conf['hardware']['gpu_num'] <= 0: - del task['Resources']['Devices'] + task["Resources"]["CPU"] = user_conf["hardware"]["cpu_num"] + task["Resources"]["MemoryMB"] = user_conf["hardware"]["ram"] + task["Resources"]["DiskMB"] = user_conf["hardware"]["disk"] + if user_conf["hardware"]["gpu_num"] <= 0: + del task["Resources"]["Devices"] else: - task['Resources']['Devices'][0]['Count'] = user_conf['hardware']['gpu_num'] - if not user_conf['hardware']['gpu_type']: - del task['Resources']['Devices'][0]['Affinities'] + task["Resources"]["Devices"][0]["Count"] = user_conf["hardware"]["gpu_num"] + if not user_conf["hardware"]["gpu_type"]: + del task["Resources"]["Devices"][0]["Affinities"] else: - task['Resources']['Devices'][0]['Affinities'][0]['RTarget'] = ( - user_conf['hardware']['gpu_type'] - ) + task["Resources"]["Devices"][0]["Affinities"][0]["RTarget"] = user_conf[ + "hardware" + ]["gpu_type"] - task['Env']['RCLONE_CONFIG_RSHARE_URL'] = user_conf['storage']['rclone_url'] - task['Env']['RCLONE_CONFIG_RSHARE_VENDOR'] = user_conf['storage']['rclone_vendor'] - task['Env']['RCLONE_CONFIG_RSHARE_USER'] = user_conf['storage']['rclone_user'] - task['Env']['RCLONE_CONFIG_RSHARE_PASS'] = user_conf['storage']['rclone_password'] - task['Env']['RCLONE_CONFIG'] = user_conf['storage']['rclone_conf'] - task['Env']['jupyterPASSWORD'] = user_conf['general']['jupyter_password'] + task["Env"]["RCLONE_CONFIG_RSHARE_URL"] = user_conf["storage"]["rclone_url"] + task["Env"]["RCLONE_CONFIG_RSHARE_VENDOR"] = user_conf["storage"]["rclone_vendor"] + task["Env"]["RCLONE_CONFIG_RSHARE_USER"] = user_conf["storage"]["rclone_user"] + task["Env"]["RCLONE_CONFIG_RSHARE_PASS"] = user_conf["storage"]["rclone_password"] + task["Env"]["RCLONE_CONFIG"] = user_conf["storage"]["rclone_conf"] + task["Env"]["jupyterPASSWORD"] = user_conf["general"]["jupyter_password"] # Submit job try: - response = Nomad.jobs.register_job({'Job': job_conf}) # noqa(F841) + response = Nomad.jobs.register_job({"Job": job_conf}) # noqa(F841) return { - 'status': 'success', - 'job_id': job_conf['ID'], + "status": "success", + "job_id": job_conf["ID"], } except Exception as e: return { - 'status': 'fail', - 'error_msg': str(e), + "status": "fail", + "error_msg": str(e), } @@ -258,7 +258,7 @@ def delete_deployment(deployment_uuid: str, authorization=security): """ # Retrieve authenticated user info auth_info = get_user_info(token=authorization.credentials) - owner, provider = auth_info['id'], auth_info['vo'] # noqa(F841) + owner, provider = auth_info["id"], auth_info["vo"] # noqa(F841) # Check the deployment exists try: @@ -267,10 +267,10 @@ def delete_deployment(deployment_uuid: str, authorization=security): raise HTTPException( status_code=403, detail="No deployment exists with this uuid.", - ) + ) # Check job does belong to owner - if j['Meta'] and owner != j['Meta'].get('owner', ''): + if j["Meta"] and owner != j["Meta"].get("owner", ""): raise HTTPException( status_code=403, detail="You are not the owner of that deployment.", @@ -279,4 +279,4 @@ def delete_deployment(deployment_uuid: str, authorization=security): # Delete deployment Nomad.job.deregister_job(deployment_uuid) - return {'status': 'success'} + return {"status": "success"} diff --git a/ai4papi/routers/v1/info.py b/ai4papi/routers/v1/info.py index c2a882b..a5f205c 100644 --- a/ai4papi/routers/v1/info.py +++ b/ai4papi/routers/v1/info.py @@ -38,7 +38,7 @@ def get_default_deployment_conf(module_name: str, authorization=security): """ # Retrieve authenticated user info auth_info = get_user_info(token=authorization.credentials) - vos = auth_info['vo'] + vos = auth_info["vo"] # Generate the conf conf = deepcopy(USER_CONF) diff --git a/ai4papi/routers/v1/modules.py b/ai4papi/routers/v1/modules.py index b295c44..0c5eb0e 100644 --- a/ai4papi/routers/v1/modules.py +++ b/ai4papi/routers/v1/modules.py @@ -36,35 +36,35 @@ @router.get("/") -@cached(cache=TTLCache(maxsize=1024, ttl=6*60*60)) +@cached(cache=TTLCache(maxsize=1024, ttl=6 * 60 * 60)) def get_modules_list(): """Retrieve a list of all modules.""" modules_url = f"{base_org_url}/deep-oc/master/MODULES.yml" r = requests.get(modules_url) catalog = yaml.safe_load(r.text) - modules = [i['module'].split('/')[-1] for i in catalog] # remove github prefix + modules = [i["module"].split("/")[-1] for i in catalog] # remove github prefix return modules @router.get("/summary") -@cached(cache=TTLCache(maxsize=1024, ttl=6*60*60)) +@cached(cache=TTLCache(maxsize=1024, ttl=6 * 60 * 60)) def get_modules_summary(): """Retrieve a list of modules with basic metadata. This will return a list of (name, title, summary, keywords). """ summary = [] - keys = ['title', 'summary', 'keywords'] + keys = ["title", "summary", "keywords"] for m in get_modules_list(): meta1 = get_module_metadata(m) meta = {k: v for k, v in meta1.items() if k in keys} # filter keys - meta['name'] = m + meta["name"] = m summary.append(meta) return summary @router.get("/metadata/{module_name}") -@cached(cache=TTLCache(maxsize=1024, ttl=6*60*60)) +@cached(cache=TTLCache(maxsize=1024, ttl=6 * 60 * 60)) def get_module_metadata(module_name: str): """Get the module's full metadata.""" # Check the module is in the modules list @@ -95,7 +95,7 @@ def get_module_metadata(module_name: str): # Format "description" field nicely for the Dashboards Markdown parser desc = [] - for aux in metadata['description']: + for aux in metadata["description"]: desc.append(aux) metadata["description"] = "\n".join(desc) # single string From 9d2ab7279c95e8bd9a607550eda018f30cf6f316 Mon Sep 17 00:00:00 2001 From: Alvaro Lopez Garcia Date: Thu, 27 Apr 2023 13:24:01 +0200 Subject: [PATCH 09/11] Fix some security errors - Use timeout for requests - Do not bind on 0.0.0.0 --- ai4papi/main.py | 12 ++++++++---- ai4papi/routers/v1/info.py | 2 +- ai4papi/routers/v1/modules.py | 6 +++--- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/ai4papi/main.py b/ai4papi/main.py index d6fbe1a..a785de2 100644 --- a/ai4papi/main.py +++ b/ai4papi/main.py @@ -1,7 +1,11 @@ """AI4EOSC Plaform API (AI4PAPI) with FastAPI.""" +import ipaddress +import typing + import fastapi from fastapi.security import HTTPBearer +import typer import uvicorn from ai4papi.routers import v1 @@ -64,12 +68,12 @@ def root( def run( - host: str = "0.0.0.0", + host: ipaddress.IPv4Address = ipaddress.IPv4Address("127.0.0.1"), # noqa(B008) port: int = 8080, - ssl_keyfile: str = None, - ssl_certfile: str = None, + ssl_keyfile: typing.Optional[pathlib.Path] = typer.Option(None), # noqa(B008) + ssl_certfile: typing.Optional[pathlib.Path] = typer.Option(None), # noqa(B008) ): - """Run the API using uvicorn.""" + """Run the API in uvicorn.""" uvicorn.run( app, host=host, diff --git a/ai4papi/routers/v1/info.py b/ai4papi/routers/v1/info.py index a5f205c..c9a780e 100644 --- a/ai4papi/routers/v1/info.py +++ b/ai4papi/routers/v1/info.py @@ -53,7 +53,7 @@ def get_default_deployment_conf(module_name: str, authorization=security): # Add available Docker tags url = f"{base_mod_url}/{module_name.lower()}/tags" try: - r = requests.get(url) + r = requests.get(url, timeout=15) r.raise_for_status() r = r.json() except Exception: diff --git a/ai4papi/routers/v1/modules.py b/ai4papi/routers/v1/modules.py index 0c5eb0e..f6b8a11 100644 --- a/ai4papi/routers/v1/modules.py +++ b/ai4papi/routers/v1/modules.py @@ -40,7 +40,7 @@ def get_modules_list(): """Retrieve a list of all modules.""" modules_url = f"{base_org_url}/deep-oc/master/MODULES.yml" - r = requests.get(modules_url) + r = requests.get(modules_url, timeout=10) catalog = yaml.safe_load(r.text) modules = [i["module"].split("/")[-1] for i in catalog] # remove github prefix return modules @@ -77,7 +77,7 @@ def get_module_metadata(module_name: str): # Find what is the default branch of the module gitmodules_url = f"{base_org_url}/deep-oc/master/.gitmodules" - r = requests.get(gitmodules_url) + r = requests.get(gitmodules_url, timeout=10) gitmodules_conf = configparser.ConfigParser() gitmodules_conf.read_string(r.text) @@ -90,7 +90,7 @@ def get_module_metadata(module_name: str): # Retrieve metadata from that branch metadata_url = f"{base_org_url}/{module_name}/{branch}/metadata.json" - r = requests.get(metadata_url) + r = requests.get(metadata_url, timeout=10) metadata = json.loads(r.text) # Format "description" field nicely for the Dashboards Markdown parser From 50e821f1d4c27f6aebf19d8cfb879b9c42749785 Mon Sep 17 00:00:00 2001 From: Alvaro Lopez Garcia Date: Thu, 27 Apr 2023 13:27:15 +0200 Subject: [PATCH 10/11] Add initial testing --- tox.ini | 1 + 1 file changed, 1 insertion(+) diff --git a/tox.ini b/tox.ini index 482b1a6..31fe1df 100644 --- a/tox.ini +++ b/tox.ini @@ -31,6 +31,7 @@ basepython = python3 deps = pytest>=7.2.1,<7.3 pytest-cov>=4.0.0,<4.1 + httpx -r{toxinidir}/requirements.txt allowlist_externals = find From 261cb088ba6c64b027e2cba3100b548a2d49de2a Mon Sep 17 00:00:00 2001 From: Ignacio Heredia Date: Thu, 27 Apr 2023 14:14:04 +0200 Subject: [PATCH 11/11] replace `assert` with `Exception` --- ai4papi/conf.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ai4papi/conf.py b/ai4papi/conf.py index 374dd6a..4360e2f 100644 --- a/ai4papi/conf.py +++ b/ai4papi/conf.py @@ -27,7 +27,9 @@ for group_name, params in USER_CONF.items(): USER_CONF_VALUES[group_name] = {} for k, v in params.items(): - assert "name" in v.keys(), f"Parameter {k} needs to have a name." - assert "value" in v.keys(), f"Parameter {k} needs to have a value." + if "name" not in v.keys(): + raise KeyError(f"Parameter {k} needs to have a name in `userconf.yaml`.") + if "value" not in v.keys(): + raise KeyError(f"Parameter {k} needs to have a value in `userconf.yaml`.") USER_CONF_VALUES[group_name][k] = v["value"]