feat: add base classes and docs #191
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #191 +/- ##
=======================================
Coverage 98.21% 98.21%
=======================================
Files 8 8
Lines 561 561
=======================================
Hits 551 551
Misses 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@sourcery-ai review |
|
I'm sorry, I don't understand the command Please use |
|
@sourcery-ai review |
|
🧙 Sourcery is reviewing your pull request! Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @JaeAeich - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
JaeAeich
changed the title
TESK, requests and model validators.TESK, requests and validators.
JaeAeich
changed the title
TESK, requests and validators.
| @@ -1,52 +1,19 @@ | |||
| """API server entry point.""" | |||
| """App entry point.""" | |||
There was a problem hiding this comment.
I cleaned this because I plan to give tesk a command line tool like accessiblity, so app.py file would contain those logic this would be good as tesk would mostly be used in pods and interacted via commands.
There was a problem hiding this comment.
did not get it, can you like some sort of document relevant to this
There was a problem hiding this comment.
I think it would be good to use command pattern here, note that the tesk would be run in containers/pods and it should be make sense that it follows a standard cli pattern. Current if you see pyproject.toml we have two console script for tesk, ie filer and taskamster, which makes it look like they are two seperate binaries/scritps.
I think all three component should be launched with an argument (upto discussion this is just an example) as that will make more sense, with #179.
# base class to implement all three component
class Command:
def execute(self):
pass
# TeskAPP
class TeskApp(Command):
def execute(self):
print("Executing API command...")
# FilerApp
class FilerAPP(Command):
def execute(self):
print("Executing Filer command...")
# TaskmasterApp
class TaskmasterApp(Command):
def execute(self):
print("Executing Taskmaster command...")
# app.py
class CommandInvoker:
def __init__(self):
self._commands = {}
def register(self, command_name, command):
self._commands[command_name] = command
def execute(self, command_name):
if command_name in self._commands:
self._commands[command_name].execute()
else:
print(f"No command registered for {command_name}")
# Usage
invoker = CommandInvoker()
invoker.register('--api', ApiCommand())
invoker.register('--filer', FilerCommand())
invoker.register('--taskmaster', TaskmasterCommand())
# Simulate CLI input
import sys
command_name = sys.argv[1] if len(sys.argv) > 1 else '--api'
invoker.execute(command_name)What do you think @uniqueg @kushagra189 ??
There was a problem hiding this comment.
I think it makes sense, but would like to hear feedback also from @lvarin and/or @jemaltahir.
Besides that, I don't think that this change belongs in this PR, therefore I'm not reviewing details for now.
|
|
||
| tesk | ||
| tesk.services |
There was a problem hiding this comment.
Why have we altered this? I see the parent model is still tesk?
There was a problem hiding this comment.
segregation in the sidebar of documentation.
| tesk.api.ga4gh package | ||
| ====================== | ||
|
|
||
| Subpackages |
There was a problem hiding this comment.
what is the difference between submodules and subpackages? Any reason why we have used subpackages here and submodules at all other places?
There was a problem hiding this comment.
If the docs are auto-generated - why do we version control them, then? Does this require you to build the docs before committing (and if so, what if you forget)? Couldn't you just auto-generate them as part of your docs deployment and leave them out of the CI? For packages I wrote and documented their APIs via Sphinx and RtD, I only kept the index.rst and configuration - the pages were then generated by RtD and were not under version control (see FOCA, for example).
Admittedly, the API docs I generated with Sphinx and RtD are pretty shitty 🤣 Still, do we really need these files here?
| .. toctree:: | ||
| :maxdepth: 4 | ||
|
|
||
| tesk.api.ga4gh.tes.base |
There was a problem hiding this comment.
why not include tesk.api.ga4gh.models here?
| @@ -10,5 +10,5 @@ ignore_missing_imports = True | |||
| [mypy-connexion.*] | |||
| ignore_missing_imports = True | |||
|
|
|||
| [mypy-foca] | |||
| [mypy-foca.*] | |||
There was a problem hiding this comment.
I remember we fixed a few things here? Can you list the issues faced here?
There was a problem hiding this comment.
from foca.config.config_parser import ConfigParser
This throws:
tesk/tesk_app.py:9: error: Skipping analyzing "foca.config.config_parser": module is installed, but missing library stubs or py.typed marker [import-untyped]
tesk/tesk_app.py:9: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
There was a problem hiding this comment.
Not sure if this is an issue that should be solved in a different PR though. Or were these errors a result of code added in this PR exclusively? If so, keep it here.
| Raises: | ||
| ValidationError: If the value is not valid. | ||
| """ | ||
| if not v: |
There was a problem hiding this comment.
Will this not violate the cause, pydantic is the authority for ensuring if null values are allowed or not. If we are writing validators underlying the same, should we not have a property to provide input if null is allowed or not?
There was a problem hiding this comment.
In addition this logic is flawed,
Cases like : enforced arrays cannot be accounted here. Eg. if not [] will also satisfy the first condition.
There was a problem hiding this comment.
we define at model level if the value is required or not, this will be ensured if not by pydantic then def by mypy. This means we should only care about writing validators for optional fields (as that would cover validation for required fields).
If a field is optional, the way pydantic validators are used is that we return value if its None.
if a model contains field as, my_array: [], in this case value is [] it is not None ie, its not optional in that case we sould validate it with the provided logic.
| @@ -1,52 +1,19 @@ | |||
| """API server entry point.""" | |||
| """App entry point.""" | |||
There was a problem hiding this comment.
did not get it, can you like some sort of document relevant to this
| app = init_app() | ||
| app.run(port=app.port) | ||
| try: | ||
| TeskApp().run() |
There was a problem hiding this comment.
hope we have tested backward compatibility here.
tesk/app.py
Outdated
| TeskApp().run() | ||
| except Exception: | ||
| logger.exception('An error occurred while running the application.') | ||
| raise |
There was a problem hiding this comment.
We should be raising appropriate runtime exception here.
There was a problem hiding this comment.
This is again material for at least three PRs:
- Base classes and docs (this PR)
- Changing entry point
- Various linter issues
Possibly two more PRs for
- Typing issues (unless they are exclusively the result of proposed code changes from the base classes).
- Validators (if we really need them, see comment)
Note that I did NOT review in detail the files that I have marked as belonging to a different PR.
Apart from that - I like the OOP - but, please have a look at my comments. I am not entirely sure whether all of that Pydantic stuff is really necessary. At least I think we could totally do without it (as we did for other services) - and if we want it, we should provide generic support for it in FOCA, because most of it smells pretty much like boilerplate to me - or things that have already been implemented by Connexion (request/response validation) or FOCA or Pydantic.
| tesk.services | ||
|
|
||
| .. toctree:: | ||
| :maxdepth: 4 | ||
| :caption: API | ||
|
|
||
| tesk.api |
There was a problem hiding this comment.
Can't you set this up so that the packages are auto-discovered? Bit tedious to keep this up-to-date, no?
Goes for all the other files as well...
There was a problem hiding this comment.
Can you explain why we don't just use Pydantic's tools for writing validators? What are your concrete use cases here?
Also, note that this may change for the next FOCA version, which will use Pydantic v2 - and validators work quite differently from v1.
Again
There was a problem hiding this comment.
This and other trivial changes may be required by linters, but they do not really fit in this PR. Open another style: PR to address these issues in bulk.
There was a problem hiding this comment.
Same as above: Include in a style: PR
| TeskApp().run() | ||
| except Exception as error: | ||
| logger.exception('An error occurred while running the application.') | ||
| raise InternalServerError( |
There was a problem hiding this comment.
Not sure if this is the right error. InternalServerError implies a 500 response and doesn't really make sense outside the request-response cycle/app context. I would just reraise error here if you really want to log the error with logging. Pretty sure that if anything happens it would dump the traceback and error messages to the logs even without the try/except.
There was a problem hiding this comment.
Again, I would much prefer to put this in another PR that focuses on this specific change (together with the proposed changes in tesk.app. Not reviewing details for now.
| ConfigNotFoundError: { | ||
| 'title': 'Configuration file not found', | ||
| 'detail': 'Configuration file not found.', | ||
| 'status': 500, | ||
| }, |
There was a problem hiding this comment.
This would very likely not be raised in app context, so it doesn't make sense to include it here.
| tesk.api.ga4gh package | ||
| ====================== | ||
|
|
||
| Subpackages |
There was a problem hiding this comment.
If the docs are auto-generated - why do we version control them, then? Does this require you to build the docs before committing (and if so, what if you forget)? Couldn't you just auto-generate them as part of your docs deployment and leave them out of the CI? For packages I wrote and documented their APIs via Sphinx and RtD, I only kept the index.rst and configuration - the pages were then generated by RtD and were not under version control (see FOCA, for example).
Admittedly, the API docs I generated with Sphinx and RtD are pretty shitty 🤣 Still, do we really need these files here?
There was a problem hiding this comment.
I suppose you are aware that Connexion validates requests and responses based on the OpenAPI schema without us having to write models? It's basically the main reason we use Connexion.
Admittedly, this predates the time of Pydantic - and I do like a good Pydantic model. However, this all may really not be necessary.
On another note, apart from your doc strings, I don't see anything TES-specific in here...
|
please check #198 |
Summary by Sourcery
This pull request introduces new base classes for the TESK API, including
TeskAppfor application initialization and running,BaseValidatorfor custom validation logic, andBaseTeskRequestfor common TES API endpoint logic. Additionally, it refactors the existing application initialization logic to utilize the newTeskAppclass.TeskAppclass to encapsulate the initialization and running of the TESK API server, extending the Foca framework.BaseValidatorclass for custom validation logic, which must be implemented by all custom validators.BaseTeskRequestclass to define common properties and methods for TES API endpoint business logic.app.pyto the newTeskAppclass.