Event history refactor #71
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@eriktaubeneck has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 44 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis update brings substantive improvements to 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 Configration File (
|
There was a problem hiding this comment.
do we need this empty file?
There was a problem hiding this comment.
|
well this is fun. pre-commit working locally but not on GH. I'll debug |
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- .github/workflows/pre-commit.yaml (1 hunks)
- .gitignore (1 hunks)
- .pre-commit-config.yaml (1 hunks)
- .pyre_configuration (1 hunks)
- pyproject.toml (2 hunks)
- sidecar/app/query/base.py (7 hunks)
- sidecar/app/query/status.py (1 hunks)
- sidecar/app/query/step.py (1 hunks)
- sidecar/app/routes/start.py (1 hunks)
- sidecar/app/routes/stop.py (1 hunks)
- sidecar/app/routes/websockets.py (1 hunks)
- sidecar/cli/cli.py (1 hunks)
- sidecar/tests/app/query/test_status.py (1 hunks)
Files skipped from review due to trivial changes (7)
- .github/workflows/pre-commit.yaml
- .gitignore
- pyproject.toml
- sidecar/app/routes/start.py
- sidecar/app/routes/stop.py
- sidecar/app/routes/websockets.py
- sidecar/cli/cli.py
Additional comments not posted (31)
.pyre_configuration (1)
4-4: LGTM!The modification of
source_directoriesto a dictionary format is correct and aligns with the JSON structure..pre-commit-config.yaml (3)
22-28: Additions forpytestconfiguration look good.The configuration for
pytestis correctly added and will enhance testing.
29-35: Additions forcoverageconfiguration look good.The configuration for
coverageis correctly added and will enhance coverage reporting.
35-35: Verify thepyre-checkconfiguration.Ensure that the modified
pyre-checkconfiguration works as expected.sidecar/app/query/step.py (1)
11-11: LGTM!The import statement for
Statusis correct and aligns with the refactoring.sidecar/app/query/status.py (1)
1-83: LGTM!The new functionality for managing status history is well-structured and aligns with the refactoring objectives.
However, ensure that the logic for adding status and managing status history works as expected.
sidecar/tests/app/query/test_status.py (10)
10-17: Fixture setup looks good.The
_status_history_fixturecorrectly sets up aStatusHistoryinstance with a temporary file path and a logger.
20-33: Fixture setup looks good.The
_full_status_history_fixturecorrectly sets up aStatusHistoryinstance with a sequence of status events.
36-46: Test function looks good.The
test_status_history_addfunction correctly tests adding status events and verifies the current status event.
49-55: Test function looks good.The
test_status_history_add_write_to_filefunction correctly tests writing status events to a file and verifies the file contents.
57-63: Test function looks good.The
test_status_history_add_load_from_filefunction correctly tests loading status events from a file and verifies the loadedStatusHistoryinstance.
65-69: Test function looks good.The
test_status_history_cannot_add_when_lockedfunction correctly tests that status events cannot be added when theStatusHistoryinstance is locked.
71-80: Test function looks good.The
test_status_history_cannot_add_lower_statusfunction correctly tests that lower status events cannot be added to theStatusHistoryinstance.
82-85: Test function looks good.The
test_status_history_current_status_eventfunction correctly tests retrieving the current status event from theStatusHistoryinstance.
88-89: Test function looks good.The
test_status_history_current_statusfunction correctly tests retrieving the current status from theStatusHistoryinstance.
92-115: Test function looks good.The
test_status_history_status_event_jsonfunction correctly tests retrieving the status event in JSON format from theStatusHistoryinstance.sidecar/app/query/base.py (15)
33-42: Method implementation looks good.The
__post_init__method correctly initializes theStatusHistoryinstance, creates status directories, and sets up logging.
55-57: Property implementation looks good.The
_log_dirproperty correctly returns the path to the log directory.
64-65: Property implementation looks good.The
startedproperty correctly checks the status to determine if the query has started.
68-69: Property implementation looks good.The
finishedproperty correctly checks the status to determine if the query has finished.
90-90: Property implementation looks good.The
statusproperty correctly retrieves the current status from theStatusHistoryinstance.
94-95: Setter implementation looks good.The
statussetter correctly adds a new status to theStatusHistoryinstance if the status has changed and is less than or equal toStatus.COMPLETE.
97-99: Property implementation looks good.The
status_event_jsonproperty correctly retrieves the status event in JSON format from theStatusHistoryinstance.
111-112: Property implementation looks good.The
stepsproperty correctly yields steps built from the query.
Line range hint
114-127:
Method implementation looks good.The
startmethod correctly initiates the steps, handles exceptions, and updates the status.
Line range hint
129-135:
Method implementation looks good.The
finishmethod correctly updates the status toStatus.COMPLETE, logs the completion, and performs cleanup.
142-146: Method implementation looks good.The
killmethod correctly updates the status toStatus.KILLED, logs the termination, and performs cleanup.
150-154: Method implementation looks good.The
crashmethod correctly updates the status toStatus.CRASHED, logs the crash, and performs cleanup.
156-162: Method implementation looks good.The
_cleanupmethod correctly resets the current step, removes the logger, and deletes the query from the dictionary.Tools
Ruff
159-162: Use
contextlib.suppress(ValueError)instead oftry-except-passReplace with
contextlib.suppress(ValueError)(SIM105)
164-166: Property implementation looks good.The
cpu_usage_percentproperty correctly retrieves the CPU usage percentage from the current step.
Line range hint
168-170:
Property implementation looks good.The
memory_rss_usageproperty correctly retrieves the memory RSS usage from the current step.Tools
Ruff
159-162: Use
contextlib.suppress(ValueError)instead oftry-except-passReplace with
contextlib.suppress(ValueError)(SIM105)
this moves all the
statusrelated elements into it's own directory, and adds tests for those elements.currently set up to compare against #70 , so review that first, and make sure to change the base before merging.
Summary by CodeRabbit
New Features
StatusHistoryfor improved tracking and management of query statuses.Bug Fixes
Statusto ensure consistency across different modules.Chores
.gitignoreto exclude.coverage*files.pytestandcoveragetools.Tests
StatusHistoryclass to ensure reliable functionality.