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.

Sign up for GitHub

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

Jump to bottom

upgrade xblock==2.0 #34398

Merged
merged 8 commits into from
Apr 8, 2024
Merged

upgrade xblock==2.0 #34398

merged 8 commits into from
Apr 8, 2024

Conversation

irtazaakram
Copy link
Member

@irtazaakram irtazaakram commented Mar 20, 2024 •
edited
Loading

@irtazaakram irtazaakram force-pushed the xblock2 branch 3 times, most recently from 5d8638f to b75139f Compare March 21, 2024 09:13
@kdmccormick kdmccormick mentioned this pull request Mar 21, 2024
Merged
@irtazaakram irtazaakram force-pushed the xblock2 branch 2 times, most recently from 22886ad to 1a04fe8 Compare March 25, 2024 06:09
@kdmccormick kdmccormick mentioned this pull request Mar 27, 2024
Closed
@irtazaakram irtazaakram closed this Apr 1, 2024
@irtazaakram irtazaakram reopened this Apr 1, 2024
@irtazaakram irtazaakram marked this pull request as ready for review April 1, 2024 13:10
@irtazaakram irtazaakram requested review from feanil and farhan April 1, 2024 13:11
feanil
feanil reviewed Apr 1, 2024
View reviewed changes
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

Generally makes sense but I'm a little unclear about when the runtime id_generator is updated. I think in general it should be left alone as it is set from the system runtime already. If there is a reason to change it, maybe you could explain why?

.github/workflows/unit-tests.yml Outdated Show resolved Hide resolved
cms/djangoapps/contentstore/helpers.py
id_generator = ImportIdGenerator(parent_key.context_key)
def_id = id_generator.create_definition(block_type, slug_hint)
usage_id = id_generator.create_usage(def_id)
runtime.id_generator = ImportIdGenerator(parent_key.context_key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we saving this as an attribute of the runtime? I don't think we want to update the runtime id_generator in perpetuity just use a different one here at import time right?

Copy link
Member Author

Choose a reason for hiding this comment

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

For this specific case runtime was using SplitMongoIdManager instead of ImportIdGenerator.

Details: Unit test failure : Raw Logs

In general id_generator should be assigned by runtime but for every case where a custom id_generator was required we have to pass it through attribute. i.e. runtime.id_generator

Copy link
Member

Choose a reason for hiding this comment

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

Ohhhh, that is interesting. I guess it makes sense--the runtime here is the general LMS/CMS runtime (CachingDescriptorSystem), so by default, its id_generator is not going to match ImportSystem's id_generator. Thanks for pointing that out @irtazaakram .

Setting it here is a little awkward, but I don't know of a better solution currently.

In order to contain the impact of this, do you think it would be reasonable to set the original id_generator to a temporary value, and then restore it? If you agree, could you make that change, and add a comment explaining why we need to do it?

original_id_generator = runtime.id_generator
runtime.id_generator = ImportIdGenerator(...)
...
runtime.id_generator = original_id_generator

Copy link
Member Author

Choose a reason for hiding this comment

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

done. 682a80a

Copy link
Member

Choose a reason for hiding this comment

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

great, ty 👍🏻

xmodule/tests/test_discussion.py Outdated Show resolved Hide resolved
@Anas12091101 Anas12091101 mentioned this pull request Apr 2, 2024
Merged
kdmccormick
kdmccormick reviewed Apr 2, 2024
View reviewed changes
Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

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

Just a couple comments @irtazaakram . Otherwise, I think this is good to go.

xmodule/tests/test_video.py Outdated Show resolved Hide resolved
cms/djangoapps/contentstore/helpers.py
id_generator = ImportIdGenerator(parent_key.context_key)
def_id = id_generator.create_definition(block_type, slug_hint)
usage_id = id_generator.create_usage(def_id)
runtime.id_generator = ImportIdGenerator(parent_key.context_key)
Copy link
Member

Choose a reason for hiding this comment

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

Ohhhh, that is interesting. I guess it makes sense--the runtime here is the general LMS/CMS runtime (CachingDescriptorSystem), so by default, its id_generator is not going to match ImportSystem's id_generator. Thanks for pointing that out @irtazaakram .

Setting it here is a little awkward, but I don't know of a better solution currently.

In order to contain the impact of this, do you think it would be reasonable to set the original id_generator to a temporary value, and then restore it? If you agree, could you make that change, and add a comment explaining why we need to do it?

original_id_generator = runtime.id_generator
runtime.id_generator = ImportIdGenerator(...)
...
runtime.id_generator = original_id_generator

@irtazaakram irtazaakram requested a review from kdmccormick April 3, 2024 11:06
@kdmccormick kdmccormick added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Apr 3, 2024
kdmccormick
kdmccormick approved these changes Apr 3, 2024
View reviewed changes
Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

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

Thanks @irtazaakram . The code looks good, assuming you can fix tests and linting.

Before merging, please either smoke-test this locally, or smoke-test with the sandbox that the open-craft-grove bot will add to your PR soon (username/password is openedx/openedx). Also please a cross-reference link to the DEPR ticket and the original xblock PR when you squash your commits.

cms/djangoapps/contentstore/helpers.py
id_generator = ImportIdGenerator(parent_key.context_key)
def_id = id_generator.create_definition(block_type, slug_hint)
usage_id = id_generator.create_usage(def_id)
runtime.id_generator = ImportIdGenerator(parent_key.context_key)
Copy link
Member

Choose a reason for hiding this comment

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

great, ty 👍🏻

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@irtazaakram
Copy link
Member Author

@kdmccormick All the tests have been successfully passed, and I've also tested it in the sandbox environment. I'm going ahead with the merge.

@irtazaakram irtazaakram merged commit a1f08d8 into master Apr 8, 2024
67 checks passed
@irtazaakram irtazaakram deleted the xblock2 branch April 8, 2024 09:30
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@feanil feanil mentioned this pull request Apr 18, 2024
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feanil feanil feanil left review comments

@kdmccormick kdmccormick kdmccormick approved these changes

@farhan farhan Awaiting requested review from farhan

Assignees
No one assigned
Labels
create-sandbox open-craft-grove should create a sandbox environment from this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

upgrade to XBlock==2.0.0
5 participants
@irtazaakram @open-craft-grove @edx-pipeline-bot @feanil @kdmccormick