Conversation
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @UsamaSadiq! 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. |
UsamaSadiq
force-pushed
the
xq-depr-fc-73-add-receiver
branch
4 times, most recently
from
172bd29 to
cf4fdd9
Compare
rehmansheikh222
force-pushed
the
xq-depr-fc-73-add-receiver
branch
from
754d0b1 to
f3b49be
Compare
UsamaSadiq
force-pushed
the
xq-depr-fc-73-add-receiver
branch
from
a060d23 to
c09b736
Compare
UsamaSadiq
requested review from
farhan,
feanil,
irtazaakram,
kdmccormick and
salman2013
as code owners
The latest version changes a method signature and so code here will need to be updated before that can land. There is already a PR to pick up that change #37528 so it doesn't seem worth it to add the constraint so just downgrade the package in this PR so we can land the rest of the updates.
* chore: Upgrade Python requirements * build: Downgrade edx-submissions for now. The latest version changes a method signature and so code here will need to be updated before that can land. There is already a PR to pick up that change #37528 so it doesn't seem worth it to add the constraint so just downgrade the package in this PR so we can land the rest of the updates. --------- Co-authored-by: Feanil Patel <feanil@axim.org>
ormsbee
left a comment
ormsbee
left a comment
There was a problem hiding this comment.
My apologies, this took longer than I thought it would. I ran into some things that I really didn't expect—I just wanted to check to make sure that I was reviewing the correct commits? It seems like there were some dev debug code left here (e.g. the Spanish logging, imports deep in the module, etc.
| 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 seems overly verbose for info-level logging.
grader_msgcan be arbitrarily long, right? Is it necessary to log the whole thing, as opposed to just capturing the score given?
There was a problem hiding this comment.
This level of logging is more verbose than necessary for info and grader_msg can indeed be very large. We’ll trim this down to only the essential fields.
| 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 it if it's not a string?
There was a problem hiding this comment.
We kept it here as we were doing double confirmation from edx-submissions and edx-platform. Now that edx-submissions has been merged and it verifies already that grader_msg is infact a string otherwise it doesn't call this external grader event, we can safely remove this check from here.
|
|
||
| log.info(f"---------------------> Received external grader score event: {signal}, {sender}, {score}, {kwargs}") | ||
|
|
||
| grader_msg = score.score_msg |
There was a problem hiding this comment.
Question: Is this a case where the event itself should be defined differently (or have an alias attribute)?
There was a problem hiding this comment.
This event is triggered from edx-submissions and it has already been merged. For this phase, we kept the event payload aligned with the legacy XQueue structure to maintain compatibility with the existing grading flow. Once we move further in the deprecation and no longer need to mirror XQueue, we can introduce a cleaner event schema or alias fields.
| 'lms_key': str(score.submission_id), | ||
| 'queue_name': score.queue_name | ||
| }), | ||
| 'xqueue_body': json.dumps(grader_msg) if isinstance(grader_msg, dict) else grader_msg, |
There was a problem hiding this comment.
As a general thing, why do we have to guess here? If it's because of backwards compatibility, please document what grader_msg can be and why somewhere in the comments. So for instance, why would it be a string vs. a dict vs. something else.
There was a problem hiding this comment.
Now that we have merged edx-submissions, we know for sure we will be receiving str in grader_msg and as we would be doing json.loads before this line of code, we are safe to run json.dumps without any if else checks.
xmodule/capa/score_render.py
Outdated
| log.exception(f"No se pudo encontrar el bloque {usage_key} en modulestore: {e}") | ||
| raise Http404(f"No se encontró el módulo {usage_key_string}") from e |
xmodule/capa/score_render.py
Outdated
| msg = f"It {usage_key_string}" | ||
| log.error(msg) |
There was a problem hiding this comment.
I'll update the error message
xmodule/capa/score_render.py
Outdated
|
|
||
| usage_key = UsageKey.from_string(usage_key_string) | ||
| course_key = CourseKey.from_string(course_id) | ||
| usage_key = usage_key.map_into_course(course_key) |
There was a problem hiding this comment.
Nit: map_into_course() shouldn't be necessary any longer, since Old Mongo courses are no longer functional for the purposes of courseware—and all SplitMongo (and later) schemes encode the course/context key into the UsageKey.
There was a problem hiding this comment.
Yes, we can remove this
xmodule/capa/score_render.py
Outdated
| usage_key = UsageKey.from_string(usage_key_string) | ||
| course_key = CourseKey.from_string(course_id) |
There was a problem hiding this comment.
Try to do these string -> key conversions at the boundaries where you receive the data into the system, i.e. the event parsing. Inner logic modules like this should try to have these key types as parameters.
xmodule/capa/score_render.py
Outdated
| log = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def load_xblock_for_external_grader(user_id, course_id, usage_key_string, course=None): |
There was a problem hiding this comment.
Please add type annotations.
|
Please rebase and squash your commits, and I'll merge. Thank you. |
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.
UsamaSadiq
force-pushed
the
xq-depr-fc-73-add-receiver
branch
from
8dfcbfa to
362904f
Compare
* chore: Upgrade Python requirements * build: Downgrade edx-submissions for now. The latest version changes a method signature and so code here will need to be updated before that can land. There is already a PR to pick up that change openedx#37528 so it doesn't seem worth it to add the constraint so just downgrade the package in this PR so we can land the rest of the updates. --------- Co-authored-by: Feanil Patel <feanil@axim.org>
6 participants
Description
edx-submissionspackage to the latest version3.12.2to test the latest changes related to event generation for XQueue submissions.