Query manager #72
Query manager #72
Conversation
initial query test, mocking env variables
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe updates enhance the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
sidecar/app/main.py (1)
4-4: Consider adding a TODO comment for future adjustments.The
QueryManageris initialized with a hardcoded limit of 1 parallel query. Since the PR summary mentions that this is designed to be adjustable in the future, adding a TODO comment can help track this task.# TODO: Make the max_parallel_queries value configurable in the future. app.state.QUERY_MANAGER = QueryManager(max_parallel_queries=1)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- .pre-commit-config.yaml (1 hunks)
- server/app/alert.tsx (2 hunks)
- server/app/query/create/page.tsx (4 hunks)
- server/app/query/layout.tsx (1 hunks)
- sidecar/app/logger.py (1 hunks)
- sidecar/app/main.py (1 hunks)
- sidecar/app/query/base.py (5 hunks)
- sidecar/app/query/ipa.py (4 hunks)
- sidecar/app/query/status.py (2 hunks)
- sidecar/app/routes/start.py (6 hunks)
- sidecar/app/routes/stop.py (3 hunks)
- sidecar/app/routes/websockets.py (3 hunks)
- sidecar/app/settings.py (2 hunks)
- sidecar/tests/app/query/test_base.py (1 hunks)
- sidecar/tests/app/routes/test_start.py (1 hunks)
Files skipped from review due to trivial changes (2)
- .pre-commit-config.yaml
- server/app/query/layout.tsx
Additional context used
Ruff
sidecar/tests/app/routes/test_start.py
57-60: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
78-79: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
96-99: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
117-118: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
Additional comments not posted (54)
sidecar/app/main.py (1)
8-8: LGTM!The inclusion of routers and middleware is correct.
sidecar/app/logger.py (2)
6-6: LGTM!The import of
get_settingsaligns with the refactoring and is correct.
9-26: LGTM!The
get_loggerfunction is well-structured and correctly configures the logger.sidecar/app/settings.py (2)
34-41: LGTM!The properties for path calculations are correctly defined and improve the readability and maintainability of the code.
43-48: LGTM!The
get_settingsfunction is well-structured and correctly memoizes the settings.sidecar/app/routes/stop.py (3)
1-1: LGTM!The import of
QueryManageraligns with the refactoring and is correct.
17-26: LGTM!The
finishendpoint is well-structured and correctly usesquery_manager.
35-42: LGTM!The
killendpoint is well-structured and correctly usesquery_manager.server/app/alert.tsx (2)
Line range hint
7-28:
LGTM! Named export is appropriate.The change to a named export for
QueryStartedAlertis appropriate and does not affect the functionality.
30-44: LGTM! New alert function is well-implemented.The
QueryFailedToStartAlertfunction is well-implemented and correctly displays an alert when a query fails to start.sidecar/app/query/status.py (1)
54-56: LGTM! Optional timestamp parameter improves flexibility.The change to make the
timestampparameter optional in theaddmethod is straightforward and improves the method's flexibility.sidecar/app/routes/websockets.py (3)
30-46: LGTM! Usingquery_managerfor query retrieval and logging is appropriate.The changes to use
query_managerfor query retrieval and logging in thestatus_websockethandler are appropriate and correct.
Line range hint
50-71:
LGTM! Usingquery_managerfor query retrieval and logging is appropriate.The changes to use
query_managerfor query retrieval and logging in thelogs_websockethandler are appropriate and correct.
78-91: LGTM! Usingquery_managerfor query retrieval and logging is appropriate.The changes to use
query_managerfor query retrieval and logging in thestats_websockethandler are appropriate and correct.sidecar/tests/app/query/test_base.py (9)
25-31: LGTM! Test case is correct.The
test_query_filestest case is straightforward and correctly verifies the existence of status and log files for aQueryinstance.
33-40: LGTM! Test case is correct.The
test_query_startedtest case is straightforward and correctly verifies thestartedproperty for different statuses of aQueryinstance.
43-50: LGTM! Test case is correct.The
test_query_finishedtest case is straightforward and correctly verifies thefinishedproperty for different statuses of aQueryinstance.
53-61: LGTM! Test case is correct.The
test_query_status_event_jsontest case is straightforward and correctly verifies thestatus_event_jsonproperty for aQueryinstance. The use ofmock_timeensures consistent timestamps.
64-71: LGTM! Test case is correct.The
test_query_runningtest case is straightforward and correctly verifies therunningproperty for different statuses of aQueryinstance.
74-79: LGTM! Test case is correct.The
test_query_managertest case is straightforward and correctly verifies theget_from_query_idmethod ofQueryManager.
82-89: LGTM! Test case is correct.The
test_query_manager_capacity_availabletest case is straightforward and correctly verifies thecapacity_availableproperty ofQueryManager.
92-105: LGTM! Test case is correct.The
test_query_manager_run_querytest case is straightforward and correctly verifies therun_querymethod ofQueryManager. The use ofmock_startensures the method is called correctly.
108-121: LGTM! Test case is correct.The
test_query_manager_run_query_at_capacitytest case is straightforward and correctly verifies therun_querymethod ofQueryManagerwhen at capacity. The use ofpytest.raisesensures the method raises the correct exception.sidecar/app/query/base.py (7)
21-23: LGTM!The
status_file_pathfunction correctly retrieves the status file path for a given query ID.
26-28: LGTM!The
log_file_pathfunction correctly retrieves the log file path for a given query ID.
33-53: LGTM!The changes to the
Queryclass improve initialization and logging. The use ofget_settingsandget_loggeris consistent with the new design.
165-166: LGTM!The
MaxQueriesRunningErrorclass is correctly defined to handle the case when the maximum number of running queries is exceeded.
170-196: LGTM!The
QueryManagerclass is well-structured and effectively manages running queries, ensuring that the number of concurrent queries does not exceed the specified limit.
183-192: LGTM!The
run_querymethod correctly handles the execution and removal of queries, ensuring capacity constraints are respected.
195-196: LGTM!The
capacity_availableproperty correctly checks if the number of running queries is below the maximum allowed.sidecar/tests/app/routes/test_start.py (7)
16-23: LGTM!The
mock_rolefixture is correctly implemented and will be useful for testing role-based functionality.
26-33: LGTM!The
running_queryfixture is correctly implemented and will be useful for testing scenarios with running queries.
36-39: LGTM!The
test_capacity_availablefunction is correctly implemented and verifies the capacity availability.
42-46: LGTM!The
test_not_capacity_availablefunction is correctly implemented and verifies the capacity availability when a query is running.
49-52: LGTM!The
test_running_queriesfunction is correctly implemented and verifies the retrieval of running queries.
55-73: LGTM!The
test_start_ipa_helperfunction is correctly implemented and verifies the starting of an IPA helper query.Tools
Ruff
57-60: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
94-112: LGTM!The
test_start_ipa_queryfunction is correctly implemented and verifies the starting of an IPA coordinator query.Tools
Ruff
96-99: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
sidecar/app/routes/start.py (7)
27-32: LGTM!The
capacity_availableendpoint is correctly implemented and uses theQueryManagerto check capacity.
35-40: LGTM!The
running_queriesendpoint is correctly implemented and uses theQueryManagerto retrieve running queries.
43-59: LGTM!The
demo_loggerendpoint is correctly implemented and uses theQueryManagerto run the query.
Line range hint
62-106:
LGTM!The
start_ipa_helperendpoint is correctly implemented and uses theQueryManagerto run the query. The role check and capacity check are correctly implemented.
113-118: LGTM!The
get_ipa_helper_statusendpoint is correctly implemented and uses theQueryManagerto retrieve the query status.
125-132: LGTM!The
get_ipa_helper_log_fileendpoint is correctly implemented and uses theQueryManagerto retrieve the query log file. The log file streaming is correctly implemented.
Line range hint
159-198:
LGTM!The
start_ipa_queryendpoint is correctly implemented and uses theQueryManagerto run the query. The role check and capacity check are correctly implemented.sidecar/app/query/ipa.py (6)
Line range hint
15-33:
LGTM!The changes to the
IPAQueryclass improve initialization and handling of kill signals. The use ofget_settingsis consistent with the new design.
Line range hint
41-58:
LGTM!The
IPACloneStepclass is correctly implemented and includes methods for building the command and handling pre-run conditions.
Line range hint
61-72:
LGTM!The
IPAUpdateRemoteOriginStepclass is correctly implemented and includes methods for building the command.
Line range hint
151-174:
LGTM!The
IPACoordinatorGenerateTestDataStepclass is correctly implemented and includes methods for building the command and handling pre-run conditions.
Line range hint
244-266:
LGTM!The
IPACoordinatorWaitForHelpersStepclass is correctly implemented and includes methods for running the step and handling termination and kill signals.
Line range hint
269-292:
LGTM!The
IPACoordinatorStartStepclass is correctly implemented and includes methods for building the command.server/app/query/create/page.tsx (4)
12-12: Import changes look good.The import statement has been correctly updated to include
QueryFailedToStartAlertand changeQueryStartedAlertto a named export.
26-28: State variable addition looks good.The new state variable
querySubmitSuccessis correctly initialized to handle form submission status.
49-56: Form submission logic changes look good.The form submission logic correctly sets
querySubmitSuccessbased on the response status.
78-83: Alert rendering logic looks good.The alert rendering logic correctly displays
QueryStartedAlertorQueryFailedToStartAlertbased on the value ofquerySubmitSuccess.
sidecar/app/logger.py
Outdated
| ) | ||
| def get_logger(): | ||
| settings = get_settings() | ||
| logger.remove() |
There was a problem hiding this comment.
how expensive is to reconfigure the logger each time this function is called? is it a mistake to call it more than once and is it also ok to call it from more than one thread?
There was a problem hiding this comment.
unclear, and easy enough to fix. I can put the logger on settings, so it's only called once. commit coming.
sidecar/app/routes/start.py
Outdated
| ): | ||
| query_manager = request.app.state.QUERY_MANAGER | ||
| if not query_manager.capacity_available: | ||
| raise HTTPException(status_code=503, detail="Capacity unavailable") |
There was a problem hiding this comment.
It would probably be nicer if we can return something like Not enough capacity to run another query: {m}/{n} queries are already running.
There was a problem hiding this comment.
return it, or have the detail in the 503 say that?
There was a problem hiding this comment.
yea in the detail I think - I assumed that detail is returned in HTTP body, so we can read it on the client side
| ): | ||
| # pylint: disable=too-many-arguments | ||
| query_manager = request.app.state.QUERY_MANAGER |
There was a problem hiding this comment.
this maybe get too repetitive and prone for errors if we need to manually check for capacity before trying to do something with query_manager.
it maybe better to add another abstraction like QueryRunner that QueryManager methods return to run a query. Methods will throw if there is no enough capacity (or return optional). At HTTP layer we could catch the exception and wrap it into a 503 error.
query_manager = QUERY_MANAGER
query_runner = query_manager.get_runner # this throws if there is no enough capacityThere was a problem hiding this comment.
it's only twice, and it's because the report collector has a different path than the helpers. if anything, we may just want to put those in the same route.
for now, I'm not too worried about a 2x duplication, but if it we're more than that, I'd agree we should handle it elsewhere.
sidecar/app/routes/start.py
Outdated
| role = settings.role | ||
| if not role or role == role.COORDINATOR: | ||
| raise Exception("Cannot start helper without helper role.") | ||
| raise IncorrectRoleError("Cannot start helper without helper role.") |
There was a problem hiding this comment.
this message could be made less ambiguous by showing the value of role variable on this helper.
sidecar/app/routes/start.py
Outdated
| role = settings.role | ||
| if role != role.COORDINATOR: | ||
| raise Exception(f"Sidecar {role}: Cannot start query without coordinator role.") | ||
| raise IncorrectRoleError( | ||
| f"Sidecar {role}: Cannot start query without coordinator role." |
There was a problem hiding this comment.
same here, we should clearly state what went wrong - starting a query on H1 instead of Coordinator, or something like that
sidecar/app/routes/stop.py
Outdated
| if query is None: | ||
| return {"message": "Query not found", "query_id": query_id} | ||
| logger.info(f"{query=}") | ||
| raise HTTPException(status_code=404, detail="Query not found") |
There was a problem hiding this comment.
these null checks and rethrowing 404 are really repetitive. I think we should wrap query manager into a class in the HTTP layer that will do it in one place, so every handler does not need to do it on its own
There was a problem hiding this comment.
updated it. I just used a wrapper function - we can wrap query manager if there end up being more things like this.
Co-authored-by: Alex Koshelev <[email protected]>
Co-authored-by: Alex Koshelev <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sidecar/app/query/base.py (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- sidecar/app/query/base.py
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sidecar/app/query/base.py (5 hunks)
- sidecar/app/settings.py (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- sidecar/app/query/base.py
- sidecar/app/settings.py
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (2)
sidecar/app/routes/stop.py (1)
22-24: Ensure consistent logging levels.The logging level used (
info) before checking the query status and after might not be consistent with the severity of the operations being logged. Consider using different logging levels based on the operation's outcome or severity.- query.logger.info(f"{query=}") + query.logger.debug(f"{query=}") # Use debug for less critical information - query.logger.info("calling query finish") + query.logger.warning("calling query finish") # Use warning if the operation is significantsidecar/app/routes/websockets.py (1)
31-47: Well-implemented WebSocket integration with query management.The integration of
QueryManagerinto the WebSocket for status updates is well done. The use of an asynchronous context manager to handle WebSocket connections is a good practice. However, consider adding more detailed error handling for the WebSocket operations.Consider implementing more granular error handling for WebSocket operations to cover scenarios like network failures or data format issues.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- sidecar/app/routes/http_helpers.py (1 hunks)
- sidecar/app/routes/start.py (6 hunks)
- sidecar/app/routes/stop.py (3 hunks)
- sidecar/app/routes/websockets.py (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- sidecar/app/routes/start.py
Additional comments not posted (1)
sidecar/app/routes/stop.py (1)
18-24: Good use of centralized query retrieval.The use of
get_query_from_query_idcentralizes the retrieval of queries, which is a good practice as it reduces code duplication and centralizes error handling. This change aligns with the previous feedback provided byakoshelev.
| async def logs_websocket( | ||
| websocket: WebSocket, | ||
| query_id: str, | ||
| ): | ||
| query = get_query_from_query_id(websocket.app.state.QUERY_MANAGER, Query, query_id) | ||
|
|
||
| async with use_websocket(websocket) as websocket: | ||
| if query is None: | ||
| logger.warning(f"{query_id=} does not exist.") | ||
| return | ||
|
|
||
| with open(query.log_file_path, "r", encoding="utf8") as log_file: | ||
| if query.finished: | ||
| logger.info(f"{query_id=} complete. sending all logs.") | ||
| query.logger.info(f"{query_id=} complete. sending all logs.") | ||
| for line in log_file: | ||
| await websocket.send_text(line) | ||
| else: | ||
| logger.info(f"{query_id=} running. tailing log file.") | ||
| query.logger.info(f"{query_id=} running. tailing log file.") |
There was a problem hiding this comment.
Refactor suggestion: Optimize log file handling in WebSocket.
The current implementation reads the log file line by line in a loop. This could be optimized by using asynchronous file reading to prevent blocking the event loop.
import aiofiles
async with aiofiles.open(query.log_file_path, mode='r', encoding='utf8') as log_file:
async for line in log_file:
await websocket.send_text(line)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sidecar/app/query/base.py (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- sidecar/app/query/base.py
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
sidecar/app/settings.py (1)
14-16: Consider removing TYPE_CHECKING block if not used.The
TYPE_CHECKINGimport is used to conditionally import theLoggerclass. However, if this import is not utilized elsewhere in a way that affects typing checks, consider removing it to clean up the code.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sidecar/app/settings.py (2 hunks)
Additional comments not posted (6)
sidecar/app/settings.py (6)
1-8: Review of imports and initial setup.The imports are correctly organized and relevant to the functionality of the settings module. The use of
__future__annotations ensures forward compatibility with future Python versions.
Line range hint
18-29: Functiongen_pathand classSettingsinitialization.The
gen_pathfunction is simple and correctly implemented. TheSettingsclass initialization includes type annotations and validators which are correctly applied. The use of_loggeras a private attribute is appropriate to prevent it from being treated as a model field by Pydantic.
50-52: Propertyloggercorrectly implemented.The
loggerproperty provides encapsulated access to the_loggerinstance, adhering to best practices in object-oriented programming.
55-59: Propertieshelperandhelpers.Both properties provide access to helper instances based on roles. The implementation is straightforward and uses the dictionary access pattern correctly.
71-76: Memoizedget_settingsfunction.The use of
lru_cacheto memoize theget_settingsfunction is a good practice to ensure that the settings are instantiated only once. The function also ensures that necessary directories are created if they do not exist, which is a robust approach to handle file system dependencies.
62-68: Propertiesstatus_dir_pathandlog_dir_path.These properties dynamically generate paths based on the
root_path. This is a flexible design allowing easy modification of directory paths. However, ensure thatroot_pathis always a valid directory path to prevent runtime errors.Verification successful
Properties
status_dir_pathandlog_dir_path.The
root_pathis assigned and used in various parts of the code, includingsidecar/cli/cli.py,sidecar/app/settings.py, andsidecar/app/routes/start.py. The assignments and usages indicate thatroot_pathis often set to default values likePath("."), and there are attempts to ensure it is a valid directory usingroot_path.mkdir(parents=True, exist_ok=True). This ensures thatroot_pathis always a valid directory path, preventing runtime errors.
sidecar/cli/cli.py: Various assignments and usages ensuringroot_pathis a valid directory.sidecar/app/settings.py: Usage in propertiesstatus_dir_pathandlog_dir_path.sidecar/app/routes/start.py: Usage in constructing repository paths.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `root_path` is always a valid directory path. # Test: Search for assignments to `root_path`. Expect: Valid directory paths. rg --type python -A 5 $'root_path'Length of output: 64
Script:
#!/bin/bash # Description: Verify that `root_path` is always a valid directory path. # Test: Search for assignments to `root_path`. Expect: Valid directory paths. rg -t py 'root_path'Length of output: 2827
sidecar/app/settings.py
Outdated
| self._logger = logger | ||
| self._logger.remove() | ||
| max_role_str_len = max(len(role.name) for role in Role) | ||
| role_str = f"{self.role.name.replace('_', ' ').title():>{max_role_str_len}}" | ||
|
|
||
| logger_format = ( | ||
| "<green>{time:YYYY-MM-DD HH:mm:ss}</green> | " | ||
| "<level>{level: <8}</level> | " | ||
| "<blue>{extra[role]}</blue> - <level>{message}</level>" | ||
| ) | ||
| self._logger.configure(extra={"role": role_str}) | ||
| self._logger.add( | ||
| sys.stderr, | ||
| level="INFO", | ||
| format=logger_format, | ||
| ) |
There was a problem hiding this comment.
Refactor logging setup in model_post_init.
The logging setup in model_post_init is extensive but could be refactored for clarity and reusability. Consider extracting the logging configuration into a separate method to improve readability and maintainability.
class Settings(BaseSettings):
...
def model_post_init(self, __context) -> None:
...
+ self.configure_logger()
+ def configure_logger(self):
+ max_role_str_len = max(len(role.name) for role in Role)
+ role_str = f"{self.role.name.replace('_', ' ').title():>{max_role_str_len}}"
+ logger_format = (
+ "<green>{time:YYYY-MM-DD HH:mm:ss}</green> | "
+ "<level>{level: <8}</level> | "
+ "<blue>{extra[role]}</blue> - <level>{message}</level>"
+ )
+ self._logger.configure(extra={"role": role_str})
+ self._logger.add(
+ sys.stderr,
+ level="INFO",
+ format=logger_format,
+ )Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self._logger = logger | |
| self._logger.remove() | |
| max_role_str_len = max(len(role.name) for role in Role) | |
| role_str = f"{self.role.name.replace('_', ' ').title():>{max_role_str_len}}" | |
| logger_format = ( | |
| "<green>{time:YYYY-MM-DD HH:mm:ss}</green> | " | |
| "<level>{level: <8}</level> | " | |
| "<blue>{extra[role]}</blue> - <level>{message}</level>" | |
| ) | |
| self._logger.configure(extra={"role": role_str}) | |
| self._logger.add( | |
| sys.stderr, | |
| level="INFO", | |
| format=logger_format, | |
| ) | |
| self._logger = logger | |
| self._logger.remove() | |
| self.configure_logger() | |
| def configure_logger(self): | |
| max_role_str_len = max(len(role.name) for role in Role) | |
| role_str = f"{self.role.name.replace('_', ' ').title():>{max_role_str_len}}" | |
| logger_format = ( | |
| "<green>{time:YYYY-MM-DD HH:mm:ss}</green> | " | |
| "<level>{level: <8}</level> | " | |
| "<blue>{extra[role]}</blue> - <level>{message}</level>" | |
| ) | |
| self._logger.configure(extra={"role": role_str}) | |
| self._logger.add( | |
| sys.stderr, | |
| level="INFO", | |
| format=logger_format, | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
sidecar/app/routes/http_helpers.py (1)
8-16: Ensure proper logging for debugging purposes.While the current implementation avoids exposing internal details in the HTTPException, consider adding logging for debugging purposes to capture the query type and other relevant information internally.
import logging logger = logging.getLogger(__name__) def get_query_from_query_id( query_manager: QueryManager, query_cls: Type[Query], query_id: str, ) -> Query: query = query_manager.get_from_query_id(query_cls, query_id) if query is None: logger.debug(f"Query of type {query_cls.__name__} with ID <{query_id}> not found") raise HTTPException(status_code=404, detail=f"Query<{query_id}> not found") return query
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- sidecar/app/routes/http_helpers.py (1 hunks)
- sidecar/app/routes/start.py (6 hunks)
- sidecar/app/settings.py (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- sidecar/app/routes/start.py
- sidecar/app/settings.py
Additional comments not posted (1)
sidecar/app/routes/http_helpers.py (1)
19-30: LGTM! The function is well-implemented.The
check_capacityfunction is well-implemented and provides useful details in the exception message.
| query_id: str | ||
| current_step: Optional[Step] = field(init=False, default=None, repr=True) | ||
| logger: loguru.Logger = field(init=False, repr=False) | ||
| _logger_id: int = field(init=False, repr=False) | ||
| logger: loguru.Logger = field(init=False, repr=False, compare=False) |
There was a problem hiding this comment.
Why use @dataclass if we need to add all these exceptions for each of the fields?
Why not just a class?
There was a problem hiding this comment.
@dataclass is just a wrapper that does some codegen to build standard methods like __init__ and __compare__. These are easy ways to define what goes in that codegen. It looks verbose, but it's much cleaner than defining all those manually.
| init=False, repr=True, default_factory=dict | ||
| ) | ||
|
|
||
| def get_from_query_id(self, cls, query_id: str) -> Optional[Query]: |
There was a problem hiding this comment.
Why have self and cls in the same function?
There was a problem hiding this comment.
cls is poorly named, let me fix that. it's meant to be the class of the Query that we are getting.
There was a problem hiding this comment.
Which are the different classes of Query?
This refactors a bunch of the implicit query management into a QueryManager class, which can look up queries and run them. With this, we now also reject queries if there is more than 1 running (hard coded right now, but built as a variable to be extended later if we so choose.)
It also adds a bunch more tests! We're now up to ~60% of the sidecar with test coverage.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores