create observer tables #1351
Closed
create observer tables #1351
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
<!-- ELLIPSIS_HIDDEN --> > [!IMPORTANT] > Add `observer_cruises` and `observer_thoughts` tables, modify `artifacts` table with new columns, indexes, and constraints. > > - **Database Schema**: > - Create `observer_cruises` table with columns `observer_cruise_id`, `status`, `organization_id`, `workflow_run_id`, `workflow_id` and foreign keys. > - Create `observer_thoughts` table with columns `observer_thought_id`, `organization_id`, `observer_cruise_id`, `workflow_run_id`, `workflow_run_block_id`, `workflow_id`, `user_input`, `observation`, `thought`, `answer` and foreign keys. > - **Artifacts Table**: > - Add columns `workflow_run_id`, `workflow_run_block_id`, `observer_cruise_id`, `observer_thought_id` to `artifacts` table. > - Add `org_workflow_run_index` and `org_observer_cruise_index` indexes to `artifacts` table. > - Add foreign key constraints to `artifacts` table for new columns. > - **Downgrade Function**: > - Update `downgrade()` to drop `observer_thoughts` and `observer_cruises` tables and remove new columns and constraints from `artifacts`. > > <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=Skyvern-AI%2Fskyvern-cloud&utm_source=github&utm_medium=referral)<sup> for a0b4c18459c7824c8084933697e948151cad8776. It will automatically update as commits are pushed.</sup> <!-- ELLIPSIS_HIDDEN -->
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on db58708 in 27 seconds
More details
- Looked at
443lines of code in9files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. skyvern/forge/sdk/artifact/manager.py:11
- Draft comment:
The import ofObserverThoughthas been removed, but ensure that all related functionality and references are also removed or refactored accordingly. - Reason this comment was not posted:
Confidence changes required:50%
The removal of theObserverThoughtimport inmanager.pyis correct since the related functionality has been removed. However, theObserverThoughtclass is still present in the codebase, which might indicate incomplete removal or refactoring.
2. skyvern/forge/sdk/artifact/manager.py:19
- Draft comment:
The_create_artifactmethod has been removed and its logic consolidated intocreate_artifact. Ensure that all previous use cases and parameters are still covered by the new implementation. - Reason this comment was not posted:
Confidence changes required:50%
Thecreate_artifactmethod inmanager.pyhas been refactored to remove the_create_artifactmethod and consolidate the logic. This is a good change for simplicity, but ensure that all previous use cases are still covered.
3. skyvern/forge/sdk/artifact/manager.py:62
- Draft comment:
Theprimary_keyparameter has been removed fromupdate_artifact_data. Ensure thattask_idis always available and used correctly, asobserver_thought_idhandling has been removed. - Reason this comment was not posted:
Confidence changes required:50%
Theupdate_artifact_datamethod inmanager.pyhas been simplified by removing theprimary_keyparameter. This change assumes thattask_idis always available, which might not be the case ifobserver_thought_idwas previously used. Verify that this assumption holds true.
4. skyvern/forge/sdk/db/models.py:170
- Draft comment:
Foreign key constraints forworkflow_run_id,workflow_run_block_id,observer_cruise_id, andobserver_thought_idhave been removed. Ensure that this does not lead to data integrity issues if these fields are still used elsewhere. - Reason this comment was not posted:
Confidence changes required:50%
TheArtifactModelinmodels.pyhas removed foreign key constraints forworkflow_run_id,workflow_run_block_id,observer_cruise_id, andobserver_thought_id. This could lead to data integrity issues if these fields are still used elsewhere.
5. skyvern/forge/sdk/db/models.py:510
- Draft comment:
ObserverCruiseModelandObserverThoughtModelhave been renamed toObserverCruiseandObserverThought. Ensure all references to these models are updated accordingly. - Reason this comment was not posted:
Confidence changes required:50%
TheObserverCruiseModelandObserverThoughtModelhave been renamed toObserverCruiseandObserverThoughtrespectively inmodels.py. This change should be reflected in all references to these models.
Workflow ID: wflow_MKxhpGsYLqbwD5bQ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to db58708 in 44 seconds
More details
- Looked at
445lines of code in9files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. skyvern/forge/sdk/artifact/manager.py:11
- Draft comment:
The removal of theObserverThoughtimport is consistent with the removal of related methods and is a good cleanup step. - Reason this comment was not posted:
Confidence changes required:0%
The removal of theObserverThoughtimport frommanager.pyandbase.pyfiles is consistent with the removal of thecreate_observer_thought_artifactmethod and related methods. This is a good cleanup step.
2. skyvern/forge/sdk/artifact/manager.py:19
- Draft comment:
The refactoring ofcreate_artifactto remove the_create_artifacthelper method simplifies the code and reduces redundancy. - Reason this comment was not posted:
Confidence changes required:0%
Thecreate_artifactmethod inmanager.pyhas been refactored to remove the_create_artifacthelper method. This simplifies the code and reduces redundancy, which is a positive change.
3. skyvern/forge/sdk/artifact/manager.py:62
- Draft comment:
The simplification ofupdate_artifact_databy removing theprimary_keyparameter and related logic makes the method more straightforward and easier to maintain. - Reason this comment was not posted:
Confidence changes required:0%
Theupdate_artifact_datamethod inmanager.pyhas been simplified by removing theprimary_keyparameter and related logic. This change makes the method more straightforward and easier to maintain.
4. skyvern/forge/sdk/artifact/models.py:62
- Draft comment:
The addition of detailed field descriptions and examples in theArtifactmodel improves documentation and usability. - Reason this comment was not posted:
Confidence changes required:0%
TheArtifactmodel inmodels.pyhas been updated to include detailed field descriptions and examples. This improves the documentation and usability of the model.
5. skyvern/forge/sdk/artifact/storage/base.py:6
- Draft comment:
Consider movingFILE_EXTENTSION_MAPto be part of theArtifactTypemodel for better organization. - Reason this comment was not posted:
Confidence changes required:30%
TheFILE_EXTENTSION_MAPinbase.pyis a dictionary mappingArtifactTypeto file extensions. The comment suggests it should be part of theArtifactTypemodel, which would be a more organized approach.
6. skyvern/forge/sdk/db/client.py:183
- Draft comment:
The removal of unused parameters increate_artifactreduces complexity and potential confusion. - Reason this comment was not posted:
Confidence changes required:0%
Thecreate_artifactmethod inclient.pyhas been updated to remove unused parameters. This is a good practice as it reduces complexity and potential confusion.
Workflow ID: wflow_fxx4gK9akTE3YBPv
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| workflow_run_block_id = Column(String, ForeignKey("workflow_run_blocks.workflow_run_block_id")) | ||
| observer_cruise_id = Column(String, ForeignKey("observer_cruises.observer_cruise_id")) | ||
| observer_thought_id = Column(String, ForeignKey("observer_thoughts.observer_thought_id")) | ||
| workflow_run_id = Column(String) |
There was a problem hiding this comment.
The workflow_run_id, workflow_run_block_id, observer_cruise_id, and observer_thought_id columns should have foreign key constraints to ensure data integrity, as mentioned in the PR description.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Add
observer_cruisesandobserver_thoughtstables, modifyartifactstable, and refactor related code for improved database schema and codebase organization.observer_cruisestable with columnsobserver_cruise_id,status,organization_id,workflow_run_id,workflow_id.observer_thoughtstable with columnsobserver_thought_id,organization_id,observer_cruise_id,workflow_run_id,workflow_run_block_id,workflow_id,user_input,observation,thought,answer.artifactstable by adding columnsworkflow_run_id,workflow_run_block_id,observer_cruise_id,observer_thought_id.org_workflow_run_index,org_observer_cruise_indextoartifactstable.artifactstable for new columns.ObserverThoughtandObserverCruiseinmanager.py,base.py,local.py,s3.py.create_artifactandupdate_artifact_datamethods inmanager.pyby removing unnecessary parameters.Artifactmodel inmodels.pyto include new fields and descriptions.downgrade()function to dropobserver_thoughtsandobserver_cruisestables and remove new columns and constraints fromartifacts.This description was created by
for db58708. It will automatically update as commits are pushed.