FC-73 XQueue add fields to submission model (Feature: ExternalGraderDetail - Gradual Migration from Xqueue System)#283
Conversation
openedx-webhooks
added
the
open-source-contribution
|
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. DetailsWhere 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. |
|
@leoaulasneo98! Looks like you're contributing on behalf of Aulasneo -- if you haven't already, please have your manager reach out to oscm@axim.org to have you added to our existing entity CLA. Thanks! |
mphilbrick211
added
the
needs test run
|
@leoaulasneo98 could you ensure you get tests passing on this PR? |
sarina
changed the title
|
Sure Sarina already checked it. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #283 +/- ##
==========================================
+ Coverage 93.53% 94.43% +0.89%
==========================================
Files 18 18
Lines 1995 2263 +268
Branches 90 95 +5
==========================================
+ Hits 1866 2137 +271
+ Misses 118 115 -3
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mphilbrick211
removed
the
needs test run
|
@leoaulasneo98 could I ask that you comment on your PR rather than via email? The alerts I'm getting are kind of hard to follow. |
Hello Sarina, yes I apologize. I received the alert by email and responded there and then I realized it was a comment in the PR. |
e0d
changed the title
e0d
changed the title
e0d
changed the title
e0d
changed the title
leoaulasneo98
force-pushed
the
XQU-15-add-fields-to-submission-model
branch
4 times, most recently
from
527d0c2 to
f1d19f4
Compare
|
@leoaulasneo98 - did your team review this ? I don't see any comments from anyone on the PR |
|
@sarina They have my go-ahead. |
|
Thanks folks! I'll review by end of day Wednesday. |
ormsbee
left a comment
ormsbee
left a comment
There was a problem hiding this comment.
I did a first pass through the ADR and API layer. I still need to take a closer look at the models, but I wanted to give this feedback as early as possible. Thank you!
| - Clearer connection between submissions and their grading results | ||
| - Improved error handling and retry capabilities | ||
| - Easier monitoring of the grading process | ||
| - Simplify the xwatcher and edx platform queue processing architecture |
There was a problem hiding this comment.
| - Simplify the xwatcher and edx platform queue processing architecture | |
| - Simplify the xqueue-watcher and edx-platform queue processing architecture |
There was a problem hiding this comment.
I already applied the correction from xwatcher to xqueue-watcher
| - First, build the new system alongside the existing one | ||
| - Test thoroughly to ensure everything works as expected | ||
| - Slowly transition from the old system to the new one | ||
| - Keep the old system running until we're sure the new one works perfectly |
There was a problem hiding this comment.
Are the details of the migration (waffle flag, how we recover if things go wrong, etc.) going to be in another ADR in edx-platform?
There was a problem hiding this comment.
Regarding the migration steps, they will indeed be proposed in the ADR of PR: openedx/openedx-platform#36258, however we've added a reference to this to clarify that the process will be detailed there.
docs/source/decisions/0001-submissionqueuerecord-gradual-migration-from-xqueue-system.rst
Outdated
Show resolved
Hide resolved
submissions/errors.py
Outdated
| """ | ||
|
|
||
|
|
||
| class ExternalGraderQueueCanNotBeEmptyError(SubmissionError): |
There was a problem hiding this comment.
Could we call this ExternalGraderQueueEmptyError? It seems like a shorter way to say the same thing...?
There was a problem hiding this comment.
Perfect, it is also applied.
submissions/api.py
Outdated
| if not event_data.get('queue_name'): | ||
| raise ExternalGraderQueueCanNotBeEmptyError("event_data must contain 'queue_name'") |
There was a problem hiding this comment.
Question: If queue_name is required in this way, why stuff it into the event_data dict instead of requiring it as an explicit function parameter?
There was a problem hiding this comment.
Ok I understand, I chose to change the name to external_grader_detail, I don't know if you think this is an adequate name?
submissions/api.py
Outdated
|
|
||
| def create_submission(student_item_dict, answer, submitted_at=None, attempt_number=None, team_submission=None): | ||
| """Creates a submission for assessment. | ||
| def create_external_grader_detail(submission, event_data): |
There was a problem hiding this comment.
Nit: You can probably get away with passing in just the submission_id so that the callers can use a Submission object if they have it, but could also just pass in submission_id if all they have is a foreign key. (You can do objects.create(submission_id=submission_id ... and it will do the right thing.
submissions/api.py
Outdated
| grader_file_name=event_data.get('grader_file_name', ''), | ||
| points_possible=event_data.get('points_possible', 1), |
There was a problem hiding this comment.
Should these be optional params to the function as well? What else goes in event_data?
There was a problem hiding this comment.
Regarding this, in a PR that we still have in draft, the files for submissions that can handle documents or images are passed. Since there are multiple parameters, we chose to use kwargs. Below I leave you a reasoning for this choice. The data that goes are the queue_name, points_possible, files and grader_file_name; for the moment it is only those.
submissions/api.py
Outdated
| submitted_at=None, | ||
| attempt_number=None, | ||
| team_submission=None, | ||
| **event_data |
There was a problem hiding this comment.
I don't think the term event_data quite captures what it's doing with respect to enabling external grading. Is there a more descriptive name we could use?
There was a problem hiding this comment.
Hi Dave, I understand the changes you've proposed and I've implemented most of them. Regarding the migration steps, they will indeed be proposed in the ADR of PR: openedx/openedx-platform#36258, however we've added a reference to this to clarify that the process will be detailed there. The only change I didn't implement and made an exception for was the handling of kwargs to pass data (formerly event_data, now external_grader_detail to maintain consistency with the work done). The reasons for this are:
1 Modularity and extensibility: By using **kwargs, we're creating a function that can evolve without the need to constantly modify it signature.
2 Backward compatibility: Modifying an existing function that's already in production by adding new explicit parameters could break current implementations. The **kwargs approach minimizes this risk.
3 Code cleanliness: Adding too many explicit parameters to a function can make it difficult to read and maintain. We aim to respect the "clean code" design principle.
4 Modular nature of the architecture: Since this change is part of a gradual migration, the flexibility offered by **kwargs is ideal for incorporating new functionalities in phases without having to redesign the API completely.
I made a mistake by selecting "Re-request review". I apologize.
There was a problem hiding this comment.
Hello Dave, good afternoon. Regarding this topic here, which is the definition of function variables. We would like to know if using kwargs represents a potential error in case this PR is merged first before the edx platform PR or vice versa. A priori it should not, however we want to know your point of view. On the other hand, in these parameters, file-type objects are sent from the edx platform, just as is done in send_to_queue for the Xqueue service, this with the purpose of saving files. However, for the moment sending files does not have a major effect, since it will be implemented in later changes. We want to know if there may be any inconvenience with passing file-type objects to create submission. We have implemented and tested this, but we would like to have another, more global point of view about these two topics to avoid potential errors.
There was a problem hiding this comment.
@ormsbee to clarify (hopefully), should we change the create_submissions api to include kwargs to send the xqueue parameters, or should we leave it as it is and store the xqueue specific params in the existing answer or student_item? The complexity of storing in answer is that it must be json serializable, and the file object might cause some problems here. On the other side, adding kwargs might increase complexity specially if it is going to be used by other types of problems in the future. What do you think?
There was a problem hiding this comment.
Let's leave answer and student_item alone, and add them in new kwargs. I think it's better to add a bit of new complexity rather than change the behavior of long-existing data that we're passing through.
leoaulasneo98
force-pushed
the
XQU-15-add-fields-to-submission-model
branch
from
a4821d0 to
67f716a
Compare
leoaulasneo98
changed the title
leoaulasneo98
force-pushed
the
XQU-15-add-fields-to-submission-model
branch
3 times, most recently
from
2dbdc96 to
9ce14ad
Compare
|
@angonz Hi, good afternoon. I've been completed the changes requested and are now ready for code review. |
angonz
left a comment
angonz
left a comment
There was a problem hiding this comment.
Ok on my side. Open the PR for review
ormsbee
left a comment
ormsbee
left a comment
There was a problem hiding this comment.
Questions and requests around the model. Thank you for your patience.
submissions/models.py
Outdated
| STATUS_CHOICES = [ | ||
| ('pending', 'Pending'), | ||
| ('pulled', 'Pulled'), | ||
| ('retired', 'Retired'), | ||
| ('failed', 'Failed'), | ||
| ] |
There was a problem hiding this comment.
Could you please use a TextChoices subclass for this? It's a little easier for type checking and translations.
submissions/models.py
Outdated
| 'retired': [] | ||
| } | ||
| submission = models.OneToOneField( | ||
| 'submissions.Submission', |
There was a problem hiding this comment.
Nit: You don't need to specify the app for a model in the same module.
submissions/models.py
Outdated
| submission = models.OneToOneField( | ||
| 'submissions.Submission', | ||
| on_delete=models.CASCADE, | ||
| related_name='queue_record' |
There was a problem hiding this comment.
Why is this different from the model name, i.e. why not make the model QueueRecord?
submissions/models.py
Outdated
| Update status and timestamp atomically | ||
| """ | ||
| if not self.can_transition_to(new_status): | ||
| raise ValueError(f"Invalid transition from {self.status} to {new_status}") |
There was a problem hiding this comment.
When raising this kind of exception, please include information about which row this is, since it makes it a bit easier to look at the database and try to understand what's going on from error log messages.
submissions/api.py
Outdated
| if not external_grader_detail.get('queue_name'): | ||
| raise ExternalGraderQueueEmptyError("external_grader_detail must contain 'queue_name'") |
There was a problem hiding this comment.
If this is a required argument, please just make it a required argument in the function definition, rather than pulling it out of **external_grader_detail.
submissions/api.py
Outdated
|
|
||
| def create_submission(student_item_dict, answer, submitted_at=None, attempt_number=None, team_submission=None): | ||
| """Creates a submission for assessment. | ||
| def create_external_grader_detail(student_item_dict, answer, **external_grader_detail): |
There was a problem hiding this comment.
Please list the variables explicitly here:
| def create_external_grader_detail(student_item_dict, answer, **external_grader_detail): | |
| def create_external_grader_detail(student_item_dict, answer, queue_name, grader_file_name="", points_possible=1): |
There was a problem hiding this comment.
I understand the value of explicitly listing parameters for clarity. I've implemented the main parameters as you suggest (queue_name, grader_file_name, points_possible), but I'd also like to maintain a **kwargs argument to handle planned future expansions.
Specifically, in upcoming PRs we'll be adding:
files: This parameter will store attached files submitted in the submission. It's crucial for external graders that evaluate code or projects where students upload multiple files.
queue_key: This is an identifier necessary for proper rendering in the XBlock. The XBlock will use this key to retrieve and display evaluation results in the user interface.
This hybrid approach would give us the best of both worlds: explicit parameters for the current API providing immediate clarity, along with the flexibility to add these additional parameters in subsequent PRs without repeatedly modifying the function signature. This would also allow us to maintain backward compatibility with code that already calls this function.
Does this approach sound reasonable? If you prefer to completely avoid using **kwargs, I can explicitly list all future parameters now, even though they won't be used until later PRs.
|
|
||
| try: | ||
| instance = ExternalGraderDetail.create_from_uuid( | ||
| submission_uuid=submission_uuid, |
There was a problem hiding this comment.
Nit: Thinking on this a bit more, since you already have a reference to the submission itself at the time of creation, you can just pass the submission or submission_id in directly. No need to do the extra lookup in this method.
Actually, I guess we don't even need the method at all. This can just invoke ExternalGraderDetail.objects.create() directly.
There was a problem hiding this comment.
Hi Dave, I understand your suggestion, but there's an important detail: the create_submission method only returns the UUID, not the submission instance or ID. If we were to use ExternalGraderDetail.objects.create() directly, we would need that direct reference.
I didn't want to modify the serializer to include the ID to avoid the risk of breaking anything. However, if you think changing the serializer is viable, I'd be happy to apply that approach. Alternatively, if you have another solution in mind, let me know and I'll implement it.
There was a problem hiding this comment.
Ah, nvm then. Please leave it as is. I applaud your inclination to avoid breaking things.
submissions/api.py
Outdated
| submitted_at=None, | ||
| attempt_number=None, | ||
| team_submission=None, | ||
| **event_data |
There was a problem hiding this comment.
Let's leave answer and student_item alone, and add them in new kwargs. I think it's better to add a bit of new complexity rather than change the behavior of long-existing data that we're passing through.
leoaulasneo98
force-pushed
the
XQU-15-add-fields-to-submission-model
branch
4 times, most recently
from
87c9fd9 to
3e9464a
Compare
leoaulasneo98
force-pushed
the
XQU-15-add-fields-to-submission-model
branch
from
3e9464a to
9258793
Compare
There was a problem hiding this comment.
⚠️ Important: This PR depends on submission structure changes implemented in edx-platform repository. Please ensure those changes are deployed before merging this PR.
This is no longer true, right? It would be safe to merge this and it would remain backwards compatible even if we bumped the current edx-platform to this?
FYI @jansenk and @blarghmatey
Hi Dave, This is no longer true. This PR can be safely merged now and would remain backwards compatible even if we updated the current edx-platform. In fact, it would be better to merge this PR first before the one in edx-platform, as edx-platform will be calling functions that don't exist yet if this isn't in master. The initial approach was to implement the external grader detail in the existing create_submission function, but we've since decided to create a new function instead. |
- Add ExternalGraderDetail model for tracking external grader submissions - Implement create_external_grader_detail function for submission creation - Add validation methods for queue_name and other required fields - Include composite index for optimized submission retrieval - Add comprehensive test suite for the new functionality - Prepare extensibility hooks for future implementations (files, queue_key)
leoaulasneo98
force-pushed
the
XQU-15-add-fields-to-submission-model
branch
from
9258793 to
72c0c9f
Compare
6 participants
FC-73 Feature: ExternalGraderDetail - Gradual Migration from Xqueue System
Description
This pull request introduces the
ExternalGraderDetailmodel as part of a gradual migration of the Open edX submission queueing system (xqueue) towards a more robust and maintainable submission processing system.Motivation
The current xqueue system requires a strategic update to:
Key Improvements
Model Enhancements
ExternalGraderDetailwith:status_time)New Fields Added
queue_name: Identifies processing queuestatus: Current submission queue statestatus_time: Unified timestamp for state trackingpullkey: Submission processing tracking keygrader_file_name: Grading file metadatagrader_reply: Grader response storagepoints_possible: Maximum possible pointsQueue Processing Improvements
SubmissionQueueManagerfor queue query processingTechnical Details
State Management
STATUS_CHOICESVALID_TRANSITIONSupdate_status()methodError Handling
SubmissionQueueEmptyErrorexternal_grader_detail.get()Testing Strategy
Comprehensive test coverage includes:
Next Phases
Planned Developments
Open edX Compliance
Adheres to Open edX standards:
Performance Considerations
BREAKING CHANGES: None. Designed to be fully compatible with existing systems.