Prompt service v2 refactor #1149
Conversation
Signed-off-by: Deepak <[email protected]>
Signed-off-by: Deepak <[email protected]>
…o fix/prompt-service-refactor
…factor Signed-off-by: Deepak <[email protected]>
Signed-off-by: Deepak <[email protected]>
Signed-off-by: Deepak <[email protected]>
…o fix/prompt-service-refactor
Deepak-Kesavan
requested review from
harini-venkataraman and
chandrasekharan-zipstack
Signed-off-by: Deepak <[email protected]>
Signed-off-by: Deepak <[email protected]>
Deepak-Kesavan
changed the title
| log_level = env.get("LOG_LEVEL", LogLevel.WARN.value) | ||
| if log_level == LogLevel.DEBUG.value: | ||
| app.logger.setLevel(logging.DEBUG) | ||
| elif log_level == LogLevel.INFO.value: | ||
| app.logger.setLevel(logging.INFO) | ||
| else: | ||
| app.logger.setLevel(logging.WARNING) |
There was a problem hiding this comment.
NIT: We could pass the log level and handle it with a simple conversion in the dictConfig call itself
|
|
||
| @health_bp.route("/health", methods=["GET"]) | ||
| def health_check() -> str: | ||
| return "OK" |
There was a problem hiding this comment.
NIT: Do you know where we use this API? Should we ensure that the responses are all of the same format as a JSON?
There was a problem hiding this comment.
Not sure if it is used anywhere. Just added it because it was there in the existing code.
prompt-service/src/unstract/prompt_service_v2/helper/lifecycle_helper.py
Show resolved
Hide resolved
There was a problem hiding this comment.
NIT: You could use this file to execute code once
- create an instance of the
app - creating a db instance maybe
- configure logging
- loading plugins .etc.
| """Get the plugin metadata by name.""" | ||
| return self.plugins.get(name, {}) | ||
|
|
||
| def initialize_plugin_endpoints(self) -> None: |
There was a problem hiding this comment.
@Deepak-Kesavan over here we expose all the plugins used. Do you think this logic can be moved into the cloud repo? Similar to how we load the plugins can we pass self.app into the plugin?
cc: @hari-kuriakose
There was a problem hiding this comment.
@chandrasekharan-zipstack It is fine because name of the plugins will be exposed anyway where it is used and it can't be avoided.
Also I am planning to handle endpoint init using blueprints for plugins in a separate PR.
| @app.before_request | ||
| def log_request_info(): | ||
| """Log request details.""" | ||
| app.logger.info(f"Request Path: {request.path} | Method: {request.method}") |
There was a problem hiding this comment.
NIT: @Deepak-Kesavan it might help to log the request payload as well maybe (as long as it doesn't contain anything sensitive)
|
|
||
| load_dotenv() | ||
|
|
||
| dictConfig( |
There was a problem hiding this comment.
Would be useful if we can log a requestID as well with a filter. Check this. Ideally we should obtain the requestID from backend / tool - but for the time being if its not there let's create one to help correlate logs within this service
| PSKeys.METRICS: metrics, | ||
| } | ||
| return response | ||
| except APIError as api_error: |
There was a problem hiding this comment.
NIT: @Deepak-Kesavan can we move such exception handling into the function being called itself rather than nesting the code further here?
| metadata[PSKeys.CONTEXT][output[PSKeys.NAME]] = ( | ||
| AnswerPromptService.get_cleaned_context(context) | ||
| ) | ||
| else: |
There was a problem hiding this comment.
NIT: Add a comment to indicate this is for chunking based calls
| f"Processing prompt type: {output[PSKeys.TYPE]}", | ||
| ) | ||
|
|
||
| if output[PSKeys.TYPE] == PSKeys.NUMBER: |
There was a problem hiding this comment.
NIT: We could move this handling based on cell type into a separate function to reduce cyclomatic complexity
…per.py Co-authored-by: Chandrasekharan M <[email protected]> Signed-off-by: Deepak K <[email protected]>
Signed-off-by: Deepak <[email protected]>
…o fix/prompt-service-refactor
|
|
What
Why
How
Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.