Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make sure parallel corpora exist before updating #565

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

johnml1135
Copy link
Collaborator

@johnml1135 johnml1135 commented Dec 11, 2024

Fix for #564


This change is Reviewable

@johnml1135 johnml1135 requested a review from ddaspit December 11, 2024 13:07
* Migrate by adding ParallelCorpora field to all projects
@johnml1135 johnml1135 force-pushed the check_parallel_copora_exists branch from f8e6250 to 8745642 Compare December 11, 2024 14:13
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.97%. Comparing base (779cb72) to head (8745642).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #565      +/-   ##
==========================================
+ Coverage   62.96%   62.97%   +0.01%     
==========================================
  Files         279      279              
  Lines       13979    13984       +5     
  Branches     1814     1814              
==========================================
+ Hits         8802     8807       +5     
  Misses       4551     4551              
  Partials      626      626              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@Enkidu93 Enkidu93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this update error coming from? I just wonder if it'd be cleaner to avoid the operation if there are no parallel corpora or otherwise change it.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ddaspit)

@johnml1135
Copy link
Collaborator Author

When a DataFile is being updated, it syncs the filename to the corpora and parallelCorpora. The old records in the database don't have parallel-corpora, and when the update is pushed, it cannot filter because the field (parallelCorpora) does not exist. This adds it on startup (migration) ensuring that the update operation can work.

Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point, we probably want to store a model version in the DB, so that we only run the migration if we need to.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @johnml1135)

@ddaspit
Copy link
Contributor

ddaspit commented Dec 11, 2024

@Enkidu93 We did not mark the new ParallelCorpora property as optional in the model in the mixed source PR.

Copy link
Collaborator

@Enkidu93 Enkidu93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, yep, that's a good point.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @johnml1135)

@johnml1135
Copy link
Collaborator Author

This is remove the migration scripts: #567

@johnml1135 johnml1135 merged commit 1ed1e57 into main Dec 11, 2024
3 of 4 checks passed
@johnml1135 johnml1135 deleted the check_parallel_copora_exists branch December 11, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants