FC-73 Implement receiver EXTERNAL_GRADER_SCORE_SUBMITTED signal to render score to student (Feature: Improve robust score rendering)#36408
Conversation
|
Thanks for the pull request, @leoaulasneo98! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
9c6d843 to
e4f1558
Compare
e4f1558 to
547c2c1
Compare
|
|
||
| course_key = CourseKey.from_string(score.course_id) | ||
|
|
||
| with modulestore().bulk_operations(course_key): |
There was a problem hiding this comment.
bulk_operations shouldn't matter for read operations on content. The intent was to delay certain metadata updates (like inheritance tree caching) and signal emission when doing a series of batched writes to content.
There was a problem hiding this comment.
I already removed this Dave, you were right it was an unnecessary operation
| # pylint: disable=broad-exception-caught | ||
| try: | ||
| # Use our new function instead of load_single_xblock | ||
| from xmodule.capa.score_render import load_xblock_for_external_grader |
There was a problem hiding this comment.
Why does this need to be a local import?
There was a problem hiding this comment.
Hi Dave, this is to avoid circular imports, since when the LMS is initialized, the rendering processes are called at the same time as the handlers. By performing internal imports, we avoid circular imports.
547c2c1 to
6fc46c5
Compare
ormsbee
left a comment
There was a problem hiding this comment.
I realize this is still in Draft mode, so some of these comments may not be relevant. Thank you.
| recalculate_subsection_grade_v3 | ||
| ) | ||
| from openedx.core.djangoapps.course_groups.signals.signals import COHORT_MEMBERSHIP_UPDATED | ||
| from openedx.core.djangoapps.signals.signals import ( # lint-amnesty, pylint: disable=wrong-import-order |
There was a problem hiding this comment.
Is there a reason we need to import it out of order here?
| log.info(f"---------------------> Received external grader score event: {signal}, {sender}, {score}, {kwargs}") | ||
|
|
||
| grader_msg = score.score_msg | ||
| log.info(f"---------------------> course_id: {score.course_id}") | ||
| log.info(f"---------------------> user_id: {score.user_id}") | ||
| log.info(f"---------------------> module_id: {score.module_id}") | ||
| log.info(f"---------------------> submission_id: {score.submission_id}") | ||
| log.info(f"---------------------> queue_key: {score.queue_key}") | ||
| log.info(f"---------------------> queue_name: {score.queue_name}") | ||
| log.info(f"---------------------> score reply: {grader_msg}") |
There was a problem hiding this comment.
This is pretty verbose logging that I would expect for debug, but not "info" level. Try to limit info-level debugging to just one or two lines per request.
| if isinstance(grader_msg, str): | ||
| try: | ||
| # Try to parse it as JSON if it's a string | ||
| grader_msg = json.loads(grader_msg) |
There was a problem hiding this comment.
What is the value we actually expect for grader_msg?
| "org.openedx.learning.external_grader.score.submitted.v1": { | ||
| "learning-external-grader-score-lifecycle": { | ||
| "event_key_field": "score.submission_id", | ||
| "enabled": Derived(_should_send_learning_badge_events) |
There was a problem hiding this comment.
Why is this bit about _should_send_learning_badge_events in here?
| path('api/notifications/', include('openedx.core.djangoapps.notifications.urls')), | ||
| ] | ||
|
|
||
| from submissions import urls as submissions_urls |
There was a problem hiding this comment.
Please group this import with the others.
| from xmodule.capa.score_render import load_xblock_for_external_grader | ||
| instance = load_xblock_for_external_grader(score.user_id, | ||
| score.course_id, | ||
| score.module_id, | ||
| course=course) | ||
|
|
||
| # Call the handler method (mirroring the original xqueue_callback) | ||
| instance.handle_ajax('score_update', data) | ||
|
|
||
| # Save any state changes | ||
| instance.save() |
There was a problem hiding this comment.
Why does this handler code live here, instead of in capa_block.py?
This commit implements a comprehensive solution for test score integration in the enhancement system along with improvements to the score rendering mechanism. Key changes include: - Add event handler for rendering blocks with edx-submissions scores - Implement event-based mechanism to render XBlocks with scoring data - Create signal handlers in handlers.py to process external grader scores - Develop specialized XBlock loader for rendering without HTTP requests - Add queue_key propagation across the submission pipeline - Register submission URLs in LMS routing configuration - Add complete docstrings to score render module for better code maintainability - Add ADR for XBlock rendering with external grader integration - Add openedx-events fork branch as a dependency in testing.in - Upgrade edx submission dependency These changes support the migration from traditional XQueue callback HTTP requests to a more robust event-based architecture, improving performance and reliability when processing submission scores. The included ADR documents the architectural decision and implementation approach for this significant improvement to the external grading workflow.
6fc46c5 to
22b50c6
Compare
[FC-73] Feature: Event-based XBlock rendering for external grader submissions
Description
This PR implements the final component in our migration from XQueue callbacks to an event-driven architecture for processing external grader submissions. It adds a specialized event handler that renders XBlocks with scoring data from edx-submissions, completing the full workflow for the code response grading system.
Key Changes:
EXTERNAL_GRADER_SCORE_SUBMITTEDsignalsscore_render.pyfor rendering blocks without HTTP requestshandlers.pyto process external grader scores from eventsxqueue_interface.pyandxqueue_submission.pyUser Impact:
This change completes the migration initiative that began with the implementation of the
ExternalGraderDetailmodel, continued with theSubmissionFilesystem andXQueueViewSet, and was extended with event emission functionality in previous PRs.Supporting information
This PR is the final component of the FC-73 initiative to modernize the external grading architecture:
ExternalGraderDetailmodel for submission state managementSubmissionFilesystem for enhanced file managementXQueueViewSetfor compatible API endpointsThe overall objective is to migrate from the traditional XQueue callback HTTP requests to a more robust event-driven architecture where edx-submissions notifies LMS about scoring events via Django signals.
Testing instructions
send_to_submission_course.enablewaffle flagEXTERNAL_GRADER_SCORE_SUBMITTEDsignalDeadline
None. This is part of the planned modernization of the external grading architecture.
Other information
Dependencies
This PR depends on all previous PRs in the FC-73 series, particularly:
ExternalGraderDetailmodel for storing submission detailsEXTERNAL_GRADER_SCORE_SUBMITTEDTechnical details
The event-driven approach provides several advantages over the HTTP callback system:
This PR maintains backward compatibility while enabling the new event-driven approach, allowing for a gradual migration of courses to the new system via the waffle flag.