PoC grades sync #6681

marcospri · 2024-09-13T07:33:04Z

Summary of the process:

/POST assignment/grading/sync
- Returns a 409 if a sync is already in progress
- Creates a new scheduled sync and starts a new job to kick off that task
/GET assignment/grading/sync
- Returns the last sync we have in the DB, finished or in progress.
Once a new sync is scheduled a celery task schedules individual celery task for each grade.
Each finished individual grading either succeeds or fails, it stores its status in the DB.
At the end of each individual sync we check if the whole task is finished and we mark it as so in the DB if that's the case.

marcospri · 2024-09-13T07:33:38Z

lms/models/grading_sync.py

+    FAILED = "failed"
+
+
+class GradingSync(CreatedUpdatedMixin, Base):


Represents a "sync"

marcospri · 2024-09-13T07:33:53Z

lms/models/grading_sync.py

+    status: Mapped[str | None] = varchar_enum(AutoGradingSyncStatus)
+
+
+class GradingSyncGrade(CreatedUpdatedMixin, Base):


Represents each of the individual grades of a sync

lms/security.py

marcospri · 2024-09-13T07:34:45Z

lms/services/auto_grading.py

@@ -52,3 +53,21 @@ def calculate_grade(

    grade = min(100, grade)  # Proportional grades are capped at 100%
    return round(grade, 2)
+
+
+def get_last_synced_grade_for_h_userids(db, h_userids: list[str], assignment_id):


Query all the previous grades from a list of students. Useful to display it on metrics endpoint.

marcospri · 2024-09-13T07:35:00Z

lms/services/lti_grading/_v13.py

@@ -87,7 +87,7 @@ def sync_grade(  # noqa: PLR0913
        self,
        lti_registration: LTIRegistration,
        lis_outcome_service_url: str,
-        grade_timestamp: datetime,
+        grade_timestamp: str,


Take the string here directly

marcospri · 2024-09-13T07:40:38Z

lms/views/dashboard/api/grading.py

+                GradingSync.status.in_("scheduled", "in_progress"),
+            )
+        ):
+            return grading_sync


TODO we actually need to serialize this.

marcospri · 2024-09-13T07:40:53Z

lms/views/dashboard/api/grading.py

+        ):
+            return grading_sync
+
+        return {}


Maybe a 404?

Yeah, I think we should error here.

marcospri · 2024-09-13T07:41:45Z

lms/views/dashboard/api/grading.py

+        if grading_sync := self.db.scalar(
+            select(GradingSync).where(
+                GradingSync.assignment_id == assignment.id,
+                GradingSync.status.in_("scheduled", "in_progress"),


We should return here the last one, whatever the status is.

marcospri · 2024-09-13T07:42:13Z

lms/views/dashboard/api/grading.py

+        renderer="json",
+        permission=Permissions.GRADE_ASSIGNMENT,
+    )
+    def get_auto_grading_sync(self):


This is the /GET, it returns the last sync.

If it's a completed one, useful to show a "last_synced_at" or similar.

If in progress, useful to report progress.

marcospri · 2024-09-13T07:42:28Z

lms/views/dashboard/api/user.py

+        users = self.request.db.scalars(users_query).all()
+
+        last_sync_grades = {}
+        if assignment.auto_grading_config:


Query that last grades synced.

acelaya

I took a first pass. The logic looks good in general. I just had some questions around the table structure.

acelaya · 2024-09-13T07:51:46Z

lms/migrations/versions/f68aacfc62c7_migration_for_grading_sync_tables.py

+    sa.Column('lms_user_id', sa.Integer(), nullable=False),
+    sa.Column('grade', sa.Float(), nullable=True),
+    sa.Column('extra', postgresql.JSONB(astext_type=sa.Text()), server_default=sa.text("'{}'::jsonb"), nullable=False),
+    sa.Column('success', sa.Boolean(), nullable=True),


Would it make sense to track a status here, like in the parent table? Would we need to know if it's in progress, or just of it succeeded or not?

Yeah we could use status instead of this, in this PR the status is:

~~Null grade & Null success, scheduled~~
~~Null grade & success = False, failed~~
~~grade & success = True, succeeded~~

This is actually simpler:

Null success, scheduled
success = true, succeed
success = false, failed

I don't have a strong opinion here. If you think using a boolean we can infer all required information, fine by me.

acelaya · 2024-09-13T07:53:22Z

lms/migrations/versions/f68aacfc62c7_migration_for_grading_sync_tables.py

+    sa.Column('id', sa.Integer(), autoincrement=True, nullable=False),
+    sa.Column('grading_sync_id', sa.Integer(), nullable=False),
+    sa.Column('lms_user_id', sa.Integer(), nullable=False),
+    sa.Column('grade', sa.Float(), nullable=True),


Why is grade nullable? Is there some case in which we want to sync with no grade?

~~Grade is null when we "schedule" the grade sync.~~

Having the rows in the DB simplifies the queries to know if we are already done, we can just query this table instead of having to store the expected result in the GradeSync table.

Oh, I thought grade would hold the value that is supposed to be synced when the corresponding task runs. But if I'm getting what you say, this value is set later, once the actual sync with the LMS has succeeded?

If that is the case, how does the celery task know what is the grade that needs to be sent to the LMS?

Yes, you are right, have done a few iterations of this locally and I was describing the previous one.

Grade should not be nullable here

acelaya · 2024-09-13T07:53:37Z

lms/migrations/versions/f68aacfc62c7_migration_for_grading_sync_tables.py

+    sa.Column('grading_sync_id', sa.Integer(), nullable=False),
+    sa.Column('lms_user_id', sa.Integer(), nullable=False),
+    sa.Column('grade', sa.Float(), nullable=True),
+    sa.Column('extra', postgresql.JSONB(astext_type=sa.Text()), server_default=sa.text("'{}'::jsonb"), nullable=False),


What is the extra field for?

Right now it just to store info about the error for failed syncs, we could just name it "error_details" or simlar.

Yeah, I would personally call it error_details, or at least result_details, that way it's more clear it's not supposed to be set until the sync has finished, and holds additional details of how it finished and why the status is the one it is.

Went for error_details.

acelaya · 2024-09-13T07:54:30Z

lms/models/grading_sync.py

+    extra: Mapped[JSONB] = mapped_column(
+        JSONB(),
+        server_default=text("'{}'::jsonb"),
+        nullable=False,
+    )


Could you add a comment of what is extra for? I think the rest are self-explanatory.

acelaya · 2024-09-13T07:57:53Z

lms/views/dashboard/api/grading.py

+                GradingSync.status.in_(["scheduled", "in_progress"]),
+            )
+        ):
+            return 409


Does this request return an empty body?

Up to us, we could return some basic "grade already in progress"

If we have some standard format for errors, then let's return the most basic one indicating the reason of the error. Otherwise, an empty body should work for now.

lms/views/lti/basic_launch.py

marcospri added 3 commits September 11, 2024 11:51

New dahboard endpoint to sync grades of a given async

2f1ebc0

Expose grade syncing endpoint to the FE

fe680b6

PoC grades sync

581ad96

marcospri commented Sep 13, 2024

View reviewed changes

acelaya reviewed Sep 13, 2024

View reviewed changes

marcospri force-pushed the sync-grades-dashbaord branch from fe680b6 to 5b10d03 Compare September 16, 2024 07:43

Base automatically changed from sync-grades-dashbaord to main September 19, 2024 08:05

marcospri closed this Sep 20, 2024

marcospri deleted the sync-grades-poc branch November 13, 2024 10:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PoC grades sync #6681

PoC grades sync #6681

marcospri commented Sep 13, 2024 •

edited

Loading

marcospri Sep 13, 2024

marcospri Sep 13, 2024

marcospri Sep 13, 2024

marcospri Sep 13, 2024

marcospri Sep 13, 2024

marcospri Sep 13, 2024

acelaya Sep 13, 2024

marcospri Sep 13, 2024

marcospri Sep 13, 2024

marcospri Sep 13, 2024

acelaya left a comment

acelaya Sep 13, 2024

marcospri Sep 13, 2024 •

edited

Loading

acelaya Sep 13, 2024

acelaya Sep 13, 2024

marcospri Sep 13, 2024 •

edited

Loading

acelaya Sep 13, 2024 •

edited

Loading

marcospri Sep 13, 2024

acelaya Sep 13, 2024

marcospri Sep 13, 2024

acelaya Sep 13, 2024

marcospri Sep 16, 2024

acelaya Sep 13, 2024

acelaya Sep 13, 2024

marcospri Sep 13, 2024

acelaya Sep 13, 2024

		FAILED = "failed"


		class GradingSync(CreatedUpdatedMixin, Base):

		status: Mapped[str \| None] = varchar_enum(AutoGradingSyncStatus)


		class GradingSyncGrade(CreatedUpdatedMixin, Base):

PoC grades sync #6681

PoC grades sync #6681

Conversation

marcospri commented Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acelaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcospri Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcospri Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

acelaya Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcospri commented Sep 13, 2024 •

edited

Loading

marcospri Sep 13, 2024 •

edited

Loading

marcospri Sep 13, 2024 •

edited

Loading

acelaya Sep 13, 2024 •

edited

Loading