docs: ADR for lightweight extension points #2182
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @mariajgrimaldi! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2182 +/- ##
=======================================
Coverage 95.04% 95.04%
=======================================
Files 191 191
Lines 21035 21035
Branches 1902 1902
=======================================
Hits 19992 19992
Misses 779 779
Partials 264 264
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks a lot for starting this @mariajgrimaldi. I left comments on the main sections but since those can disapear once we change the code I'll leave a short summary here.
The current form of the ADR is focused on the use case we have at hand and I believe that is less important than the goal of moving the needle from the world we leave now where extending ORA requires modifying the core code, which is difficult and creates maintenance issues. I propse we the ADR on the advocacy for a more extensible ORA using the existing Hooks Extensions Framework, allowing for external interactions with minimal code changes so that the level of effort is something we can commit to in the near future.
|
|
||
| **Draft** *2024-02-27* | ||
|
|
||
| Context |
There was a problem hiding this comment.
I think the context described here is focusing too much on the particular integration that we are looking to build on top of an extension mechanism. Given the experience we have had trying to modify ORA2 so far the issue is that the needs of different users like edx.org, the spanish universitites, other on campus intiatives and other members of the community don't align.
I propose we focus the ADR towards the issues created by not having any extension mechanism that does not require forking the code. By this I mean, ORA is in itself designed to be extended, it is possible to create new steps and configure them in, however this requires essencially modifiying the core of the ORA repo a.ka. forking it.
We have at least two examples one being the code we wrote for the spanish campus uses and the other being RG's ai-ora2 work.
I propose something like this for the context:
Open Responses are commonly used in education for assessment purposes and the flexibility of ORA has made it a key feature of the Open edX platform. However as developers it is very hard to accomodate the needs of educators and other stakeholders in the teaching process when they want to enhance the learners experience with other tools such as plagiarism detection, AI grading, coding graders and others.
As the code is open it is always possible to modify it incurring in a technical mainteinance debt that for the most part either prevents educators from actually experimenting or prevents the instances to upgrade to newer versions once the changes to ORA become important in the teaching process.
There was a problem hiding this comment.
I see. I already included the suggestions. Thanks for the feedback!
| For the first extension point we will use an `Open edX Filter`_ with the following definition: | ||
|
|
||
|
|
||
| .. code-block:: python |
There was a problem hiding this comment.
To make the ADR more readable I suggest we move the code to a second file with code examples that we can reference from this text, but keep the conceptual part as the center of this file.
There was a problem hiding this comment.
I was researching a bit about where the code for the proposal should live and found this:
Architecture Decision Records (ADRs) are local lightweight documents of technical decisions co-located with their corresponding code...
So that's why I'm trying to illustrate what this will look like, so we do this or use the implementation instead as an illustration. I propose removing the code altogether without adding a new file, and then, if needed, we add references to the actual code implemented. If you believe adding the new file is the way to go, I'll do it. Let me know! Thanks
|
|
||
| Extension developers could interact with an essential part of the student's assessment lifecycle with these changes. But when none of these extension points are configured for use, then ORA assessments will behave as usual. | ||
|
|
||
| Rejected Alternatives |
There was a problem hiding this comment.
Given that there is currently no other option for extending ORA without a fork we are not really rejecting any other alternative at this point. It could be argued that we are rejecting the construction of a larger (or more comprehensive) framework for extension but I don't see it like that. It is more like this is the first step towards a larger framework. If we were to propose a project to extend ORA with a mechanism for dependency injection we would still propose it to be build on top of the hooks framework.
At this ADR we are only commiting to the first few hooks because we understand very well the effort that it requires. However there is no technical limit for this proposal to grow into more hooks and to eventually support a broad array of extension use cases. Maybe this reflection is more for the consequences part. We are opening the door to more extension points in the future that are also built using events and filters and we can make it more explicit.
There was a problem hiding this comment.
I was wondering what to add here, so I'll use this comment instead. Thanks.
There was a problem hiding this comment.
I suppose an alternative approach would be to make adding grading steps much more robust, using inversion of control and dependency injection to add steps rather than forks. That would likely be significantly more work and would add another divergent pattern to events which we are using elsewhere. A couple of general questions. Could we imagine a solution where an event could be added to a function without changing the code? In other languages than python you might do this with aspects, AOP point cuts or similar. Not sure this is a good idea, so not advocating for it currently, but it would be extremely flexible.
There was a problem hiding this comment.
Could we imagine a solution where an event could be added to a function without changing the code?
That's an approach that we haven't explored yet. The closest example I've seen is how people have implemented filters as decorators when used in multiple places (here's an example: https://github.com/openedx/event-routing-backends/blob/master/event_routing_backends/processors/openedx_filters/decorators.py#L9). This approach looks cleaner and is easier to maintain than duplicating code. However, I think we might be missing the explicitness of what's been processed and where for more complex functions. I imagine there's a way to solve that. It would be interesting to look further into it!
|
Hi @felipemontoya, thanks for the review. I'll be including your suggestions as soon as possible. |
|
I believe I addressed all your comments @felipemontoya. Could you check again? Thanks. |
|
Hello folks, @pomegranited, @GlugovGrGlib, @e0d. I'm tagging you here as candidates to review this extension proposal if you can do so. The PR cover letter has all the info you might need; feel free to ask for any clarification. Thanks! |
itsjeyd
added
core contributor
|
Very interesting thank you @mariajgrimaldi ! I'll take a closer look next week. |
| Decisions | ||
| ********* | ||
|
|
||
| As the first step towards making implementing new use cases during the ORA user's lifecycle more flexible, we will introduce two extension points in the ORA codebase. These extension points will be built on top of the `Hooks Extensions Framework`_. The definitions for these extension points will reside in `openedx-filters`_ and `openedx-events`_. These definitions will be imported into the edx-ora2 repository and triggered in two places during the learners' lifecycle, resulting in minimal modifications to the ORA implementation. |
There was a problem hiding this comment.
What's not clear to me here is how invasive adding extension points is. Know the little I do, I assume it's just adding some code to fire events when specific methods are called and state changes occur. Is that right? It would be useful for the initiate to know how much core change is required for each extension.
There was a problem hiding this comment.
Since the idea is to maintain the previous behavior when the extension points are not in use, adding them shouldn't be invasive. We have a working implementation in one of our environments, so I'll use those as examples. Adding new extension points in this case would mean:
First, we'll propose a new Open edX Event in the openedx-events repository, similar to this one:
# .. event_type: org.openedx.learning.ora.submission.created.v1
# .. event_name: ORA_SUBMISSION_CREATED
# .. event_description: Emitted when a new ORA submission is created
# .. event_data: ORASubmissionData
# Warning: This event is currently incompatible with the event bus, list/dict cannot be serialized yet
ORA_SUBMISSION_CREATED = OpenEdxPublicSignal(
event_type="org.openedx.learning.ora.submission.created.v1",
data={
"submission": ORASubmissionData,
},
)
Where ORASubmissionData is the event payload:
@attr.s(frozen=True)
class ORASubmissionData:
"""
Attributes defined to represent event when a user submits an ORA assignment.
Arguments:
id (str): identifier of the ORA submission.
file_downloads (List[dict]): list of related files in the ORA submission. Each dict
contains the following keys:
* download_url (str): URL to download the file.
* description (str): Description of the file.
* name (str): Name of the file.
* size (int): Size of the file.
"""
id = attr.ib(type=str)
file_downloads = attr.ib(type=List[dict], factory=list)
Then we'll include the event into ORAs flow. We're proposing sending the event after a student submits a response to the assessment, here, as specified in the ADR. It'd look something like this:
file_downloads = OpenAssessmentBlock.get_download_urls_from_submission(
submission
)
ORA_SUBMISSION_CREATED.send_event(
submission=ORASubmissionData(
id=submission.get("uuid"),
file_downloads=file_downloads,
)
)
When no plugin explicitly listens to the event, the student submission will end as usual. If a plugin is configured to do so, it will execute its event receiver. If, for some reason, sending the event fails, given the definition of an Open edX Event, the application flow will not be affected.
Now, it's a similar process for the filter. First, we propose the filter definition in the openedx-filters repository:
class ORASubmissionViewRenderStarted(OpenEdxPublicFilter):
"""
Custom class used to create ORA submission view filters and its custom methods.
"""
filter_type = "org.openedx.learning.ora.submission_view.render.started.v1"
class RenderInvalidTemplate(OpenEdxFilterException):
"""
Custom class used to stop the submission view render process.
"""
def __init__(self, message: str, context: Optional[dict] = None, template_name: str = ""):
"""
Override init that defines specific arguments used in the submission view render process.
Arguments:
message (str): error message for the exception.
context (dict): context used to the submission view template.
template_name (str): template path rendered instead.
"""
super().__init__(message, context=context, template_name=template_name)
@classmethod
def run_filter(cls, context: dict, template_name: str):
"""
Execute a filter with the signature specified.
Arguments:
context (dict): context dictionary for submission view template.
template_name (str): template name to be rendered by the student's dashboard.
"""
data = super().run_pipeline(context=context, template_name=template_name, )
return data.get("context"), data.get("template_name")
We propose to execute the filter before rendering the student legacy view, here. It'd look something like this:
if path == "legacy/response/oa_response.html":
try:
# .. filter_implemented_name: ORASubmissionViewRenderStarted
# .. filter_type: org.openedx.learning.ora.submission_view.render.started.v1
context, path = ORASubmissionViewRenderStarted.run_filter(context, path)
except ORASubmissionViewRenderStarted.RenderInvalidTemplate as exc:
context, path = exc.context, exc.template_name
When the filter is not explicitly configured to execute a pipeline, the application will behave as usual. When configured, it will execute the list of functions specified by the extensions developer. Now, if, for some reason, the filter pipeline fails with an unwanted runtime error, no error will be raised. If the developer explicitly raises an exception given X condition, then the pipeline will fail raising an error in the rendering.
There was a problem hiding this comment.
This is very helpful, but I'm not sure I understand what purpose the filter plays here. Is it to display the result from TurnItIn if it exists to the instructor grading the work?
There was a problem hiding this comment.
A warning must be displayed before sharing information with external tools so the user knows what's being submitted will be shared with another service. In this case, this is done by implicitly accepting an EULA agreement before sending the information. Now, if the user disagrees with it, it'd be enough not to submit their assessment. We're adding this warning by modifying the ORA template in a plugin:
But this filter could be used for another type of use case where the submission view needs modification or extra functionality; we went for implicit compliance for time constraints, but it could be something even more complex.
|
|
||
| The first extension point we will implement is an `Open edX Filter`_ that will be executed before `rendering the submission HTML section of the block for the legacy view`_, with input arguments ``context`` and ``template path``. This implementation will allow us to modify what's rendered to the student, via the view ``context`` and ``template``, for cases when needed. | ||
|
|
||
| The second extension point to be implemented is an `Open edX Event`_. It will be sent `after a student submits a response to the assessment`_ with the student's submission key data, like the ORA submission ID and files uploaded in the submission, as the event's payload. This event (that works like a notification) will allow us to take action after a submission is made based on the data sent. |
There was a problem hiding this comment.
Are copies of the files included in the event or URLs?
There was a problem hiding this comment.
In this comment, I mentioned what we plan on using as the events payload. Is it worth adding to the ADR the reference implementation I mentioned in the comment?
There was a problem hiding this comment.
I can see how the first pre-submission filter gives the opportunity to inject a EULA statement, which we assume was seen by the student before they submit. And I can see how the second "submission created" event gives the chance to pass that submission on to an external assessment tool like Turnitin. Ok.
But shouldn't the results of this assessment should be recorded as part of the ORA assessment pipeline? How do instructors see which submissions were flagged by the external assessment tool? Do they have to view the similarity report for each submission and use that when making their Staff assessment? If so, I assume this is done by extending the ORA2 Grading MFE to support showing links to the Turnitin plugin's API views, so that's a third extension point.
Each of these integration points seem OK to add -- they are indeed lightweight, and won't affect other users of ORA2. But I'm having trouble agreeing that all together, they are the best way to implement such a broadly useful feature.
Is it very difficult to define a new "External Tool" assessment step in the ORA2 pipeline that could work for Turnitin and other external tools?
There was a problem hiding this comment.
Thanks for reviewing this.
But shouldn't the results of this assessment should be recorded as part of the ORA assessment pipeline?
There is some nuance to that. Every tool is different and the tool we have implemented (turnitin) does not give you an assesment, what it does is create a report saying "from our analysis this much is original and this much is not" which requires a human evaluator to look at this. The similarity report would for instance flag as "not original" a piece of text you are making a citation from, which is a completely valid thing to do.
How do instructors see which submissions were flagged by the external assessment tool? Do they have to view the similarity report for each submission and use that when making their Staff assessment? If so, I assume this is done by extending the ORA2 Grading MFE to support showing links to the Turnitin plugin's API views, so that's a third extension point.
What we are working towards is that this report is available for authors during the grading step in the MFE. That would indeed be another extension point. This time in the ora-grading-MFE. We are working towards having frontend-pluggability in there, as part of the effort that Adolfo is leading together with 2U in the frontend-plugin-framework repo.
But I'm having trouble agreeing that all together, they are the best way to implement such a broadly useful feature.
Is it very difficult to define a new "External Tool" assessment step in the ORA2 pipeline that could work for Turnitin and other external tools?
It is quite a lot of work yes. Specially trying to make that step configurable and flexible for different and varied use cases. For instance an "external tool assessment step" would not be helpful for our usecase as turnitin does not give an assessment, it only presents the staff user extra info to make an informed choice about the grade they will grant.
We initially started down that path by doing several POCs PRs about different things we wanted to extend ora with. We also studied an implementation RG made for an AI grader. Sadly we could not take on that much scope in the context we are at and that is when we decided to propose something very lightweight to get started. Naturally this 2 hooks won't be enough for all cases, but we can build more such hooks incrementally instead of leaving ora more time without an extensibility mechanism.
There was a problem hiding this comment.
@felipemontoya Thank you for talking me through your use case and the various other paths attempted here.
It is quite a lot of work yes. Specially trying to make that step configurable and flexible for different and varied use cases.
I'd like that to be a goal, but I understand that resources are limited. Maybe once we have some more use cases, then we can look at bringing some of your plugin implementation into the ORA2 core?
There was a problem hiding this comment.
@mariajgrimaldi Do you mind to mention "contributing an External Tool step to ORA2" as a rejected alternative here due to time constraints?
Is it worth adding to the ADR the reference implementation I mentioned in the comment?
Could you add a link to your platform plugin? I think anyone who goes searching for Turnitin support in ORA2 will want to use it, so it would be great if they can find it easily from this ADR.
Otherwise this looks good to me :)
There was a problem hiding this comment.
Absolutely. I would like to pursue the goal of having more flexibility in ora, but it was important to acknowledge now that we can't pursue it at the moment in the context of the turnitin extension we are writing for the spanish universities. However we wanted to make the ADR to discuss the posibility of bringing some extensibility now, even as we call it, a lightweight extension.
There was a problem hiding this comment.
There is some nuance to that. Every tool is different and the tool we have implemented (turnitin) does not give you an assesment, what it does is create a report saying "from our analysis this much is original and this much is not" which requires a human evaluator to look at this. The similarity report would for instance flag as "not original" a piece of text you are making a citation from, which is a completely valid thing to do.
So I'm clear, this means that this approach, events and filters, cannot be used for external tools that do return a grade and might need to block the ORA workflow if the result warranted that?
I understand the scope and complexity concerns here. This solution is nice in that it is plugable. However, if it can't support grading or interacting with the overall workflow I want to be explicit about that.
There was a problem hiding this comment.
With these two hooks, feedback for the user cannot be easily done. At least they weren't designed for it. I already ncluded it in the ADR. Thank you!
However, way back I did a rough POC that considered giving feedback to the user:
@togglable_mobile_support
def student_view(self, context=None): # pylint:
try:
self.update_workflow_status()
except AssessmentWorkflowError:
# Log the exception, but continue loading the page
logger.exception('An error occurred while updating the workflow on page load.')
ui_models = self._create_ui_models()
# All data we intend to pass to the front end.
context_dict = {
"title": self.title,
"prompts": self.prompts,
"prompts_type": self.prompts_type,
"rubric_assessments": ui_models,
"show_staff_area": self.is_course_staff and not self.in_studio_preview,
}
template_name = "openassessmentblock/oa_base.html"
try:
# .. filter_implemented_name: ORAStudentViewRenderStarted
# .. filter_type: org.openedx.learning.dashboard.render.started.v1
template_name, context_dict = ORAStudentViewRenderStarted.run_filter(
template_name=template_name,
context=context_dict
)
except ORAStudentViewRenderStarted.RenderInvalidTemplate as exc:
template_name, context_dict = (exc.template, exc.template_context)
except ORAStudentViewRenderStarted.RenderCustomFragment as exc:
fragment = exc.fragment
else:
template = get_template(template_name)
fragment = self._create_fragment(template, context_dict, initialize_js_func='OpenAssessmentBlock')
template = get_template(template_name)
return self._create_fragment(template, context_dict, initialize_js_func='OpenAssessmentBlock')
class ORAStudentViewRenderStarted(OpenEdxPublicFilter):
filter_type = "org.openedx.learning.dashboard.render.started.v1"
class RenderInvalidTemplate(OpenEdxFilterException):
def __init__(self, message, dashboard_template="", template_context=None):
super().__init__(
message,
dashboard_template=dashboard_template,
template_context=template_context,
)
class RenderCustomFragment(OpenEdxFilterException):
def __init__(self, message, fragment=None):
super().__init__(
message,
fragment=fragment,
)
@classmethod
def run_filter(cls, context, template_name):
data = super().run_pipeline(context=context, template_name=template_name)
return data.get("context"), data.get("template_name")
With that filter, we change the context and the rendered template (openassessmentblock/oa_base.html). Thus, we can add a button with a handler that calls Turnitin with the response value.
The template path we will add will be a template that lives in a platform-plugin-turnitín. This plugin will have the same content as the original template (openassessmentblock/oa_base.html), but we will add the Check with Turnitin button, which, when pressing it, will send the information to turnitín. Here you can see the button:
When clicked, an HTTP request will be made to Turnitín with the content of my answer. Then, a mechanism should be able to get the info to show it to the user. I didn't get to that, though.
itsjeyd
removed
the
waiting for eng review
pomegranited
added
the
waiting on author
|
Hi @mariajgrimaldi :) FYI I'm only waiting on this comment to be addressed, and then this is good to merge. |
|
@pomegranited @e0d: Thanks for the patience. I'll address your comments later today or early tomorrow. Thanks again! |
mariajgrimaldi
force-pushed
the
MJG/extension-points-adr
branch
from
c4d7277
to
5b809c8
Compare
There was a problem hiding this comment.
👍 @mariajgrimaldi Thank you for sharing the conclusions of your research work in this ADR, and for coming up with such a nice, lightweight way to provide this functionality.
Just need to mark this ADR as "Approved", and I'll merge.
-
I tested thisN/A - I read through the ADR and checked the links.
| Status | ||
| ****** | ||
|
|
||
| **Draft** *2024-02-27* |
There was a problem hiding this comment.
Before merge: mark this as Accepted.
|
I marked the ADR as accepted, @pomegranited; thank you for the review and all the help! |
mariajgrimaldi
force-pushed
the
MJG/extension-points-adr
branch
2 times, most recently
from
b15dffd
to
1919b90
Compare
mariajgrimaldi
force-pushed
the
MJG/extension-points-adr
branch
from
1919b90
to
d415ccd
Compare
mariajgrimaldi
force-pushed
the
MJG/extension-points-adr
branch
from
1d79dcb
to
5c0f58a
Compare
|
@mariajgrimaldi 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
itsjeyd
removed
the
waiting on author
TL;DR - This PR adds an architectural decision record (ADR) justifying adding lightweight extension points in ORA during the submission lifecycle to solve this feature request: https://openedx.atlassian.net/wiki/spaces/COMM/pages/3891855361/Assessment+step+with+external+tools+-+Turnitin
The ADR talks about sharing students' submission information with external services; in the feature request, that external service is Turnitin, a plagiarism tool.
What changed?
Developer Checklist
Testing Instructions
You could follow these instructions if you want to test what's on the ADR. They are tailored to be used with the Turnitin tool, so you'll need credentials to integrate with it:
Then, as a student, upload an assignment. Then, with the assignment ID, as an instructor, access the Turnitin API. Please use a bearer token as the authentication method as usual with Open edX APIs.
Generate similarity report for a submission given its submission ID:
PUT https://{{lms_domain}}/platform-plugin-turnitin/{{course_id}}/api/v1/similarity-report/{{submission_id}}/
Get similarity report status for a submission given its submission ID:
GET https://{{lms_domain}}/platform-plugin-turnitin/{{course_id}}/api/v1/similarity-report/{{submission_id}}/
Get viewer URL for the report given the submission ID:
GET https://{{lms_domain}}/platform-plugin-turnitin/{{course_id}}/api/v1/viewer-url/{{submission_id}}/
Reviewer Checklist
Collectively, these should be completed by reviewers of this PR:
FYI: @openedx/content-aurora