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
Creates voiceover migration job #20165
Conversation
…r_voice_artists
…r_voice_artists
…r_voice_artists
…r_voice_artists
…r_voice_artists
…r_voice_artists
…r_voice_artists
…r_voice_artists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one comment, everything else looks good. Thanks @Nik-09!
core/domain/voiceover_services.py
Outdated
) -> voiceover_models.EntityVoiceoversModel: | ||
"""Creates an entity voiceovers model instance. | ||
"""Creates an entity voiceovers model instance without putting it into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creates --> Creates and returns
Put a comma after "instance"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
core/domain/voiceover_services.py
Outdated
""" | ||
entity_voiceovers_dict = entity_voiceovers.to_dict() | ||
|
||
entity_id = entity_voiceovers_dict['entity_id'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per this comment: #20165 (comment)
I see, this is an interesting use case for to_dict().
I think the part I'm confused about is why we can't write entity_id = entity_voiceovers.id
or similar, and same for a bunch of the others below. I suggest you use the domain object where possible, and use the to_dict() for voiceovers_mapping only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Signed-off-by: Nik-09 <nikhil.agarwal.2019@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @seanlip I have addressed the comments, PTAL once you are free.
Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Nik-09! LGTM.
@chris7716 @JayVivarekar Could one of you please put a review hold on this for the server admin test of the job? Thanks. |
@Nik-09 Just to check: why is the second checkbox in "Testing doc (for PRs with Beam jobs that modify production server data)" ticked? Has the job been run on the server? The PR only just got approved so I'm a bit surprised by that. |
Hi @Nik-09, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks! |
Unticked. |
I have completed the job run in the backup server. @seanlip can you please verify the data in the datastore and merge this PR. Thank you |
Datastore looks good; merging. Thanks! |
Overview
The PR creates beam job for voiceover migration i.e., copying the exiting voiceover data from curated exploration (ExplorationModel) to new storage system for voiceovers (EntityVoiceoversModel).
Note: Due to manual dependency on voiceover admins i.e., they should assign accents to voice artists before EntityVoiceoversModel creation. Hence we can’t automatically create EntityVoiceoversModel when a new voiceover is added, after the voiceover migration job runs (this PR).
One solution we are accepting is to make the UI ready for creators and learners (upcoming PR) so that after a successful run of this job, the release coordinator enables the feature flag so that all later voiceovers will be directly added to EntityVoiceoversModel instead of ExplorationModel.
This job is safe to merge because successively running the job updates the old data that are stored in the EntityVoiceoversmodel.
Some of the post-migration scenarios are discussed in this document making this PR safe to merge.
Essential Checklist
Please follow the instructions for making a code change.
Testing doc (for PRs with Beam jobs that modify production server data)
Proof that changes are correct
voiceover_migration.mp4
Proof of changes on desktop with slow/throttled network
Proof of changes on mobile phone
Proof of changes in Arabic language
PR Pointers