Repository navigation
Refactoring points calculation code #888
Description
(From #880, delayed feedback)
There are several different places that find the final submission (best/last/marked as final) for one or many students. We should try to find a unified way of doing this, or at least reduce the number of different implementations. Especially now that the "delayed feedback" and "mark as final" updates have made the logic of finding the final submission much more complicated.
Here are the alternative implementations:
annotate_submitter_points
a-plus/exercise/submission_models.py
Line 31 in 8a852f9
- considers delayed feedback
- considers grading mode
- considers mark as final
- works with multiple users at a time
- works with multiple exercises at a time
- not cached
UserExerciseSummary
a-plus/exercise/exercise_summary.py
Line 13 in 8a852f9
- does not consider delayed feedback (caller is required to filter points)
- considers grading mode
- considers mark as final
- works with one user at a time
- works with one exercise at a time
- not cached
CachedPoints
a-plus/exercise/cache/points.py
Line 37 in 8a852f9
- considers delayed feedback
- considers grading mode
- considers mark as final
- works with one user at a time
- works with multiple exercises at a time
- cached
filter_best_submissions
- considers delayed feedback
- considers grading mode
- considers mark as final
- works with multiple users at a time
- works with multiple exercises at a time
- not cached
Activity
lainets commentedon Jun 14, 2023
CachedPointsis used for these things (mostly the first one):UserExerciseSummaryis used for these things (mostly the first one):annotate_submitter_pointsjust annotates the total points for an exercise (i.e. finds the best submission) and is used for the following:CourseAggregateDataViewSetfilter_best_submissionsis only used once in CourseSubmissionDataViewSet.Based on these, I propose that we store each exercise's data separately in the cache instead of having a single dump of the whole instance. The instance would also be cached but it would only contain the ids of the exercises instead of the actual data. If one needs both the exercises of an instance, they can just fetch twice: first to get the instance and then to get the exercises. Doing two fetches seems to be a normal thing for storing collections in memcached and should only have a trivial effect on performance.
This method would make
UserExerciseSummarytrivial to consolidate in to the cache and works well with howCachedPointsis used currently. WhetherCourseSubmissionDataViewSetcould be implemented using the cache instead offilter_best_submissionsrequires a bit more thinking but at the very least we should be able to make a singlebest_submissionsfunction that can be used by both the cache andCourseSubmissionDataViewSet.annotate_submitter_pointswould be harder to replace as it works in SQL but I'd take a look at implementing the three use cases using the cache instead. It might even make all results page faster and simpler.Also, we wouldn't need to invalidate everything after every small change. E.g. a submission would only invalidate the one exercise and the instance instead of all exercises and the instance.
markkuriekkinen commentedon Jun 20, 2023
@lainets Sounds good! To elaborate, does the course instance points cache item include any user-specific data or just the exercise ids? Does one exercise points cache item include one user's points in one exercise?
Querying the exercise points cache is easy with the exercise id and the user id, right?
lainets commentedon Jun 20, 2023
The current code calculates some overall stats for the user that are stored at the instance level. Only caching the exercises would make the resulting architecture simpler but we'll have to test if calculating the aforementioned stats on demand is too slow.
Yes
markkuriekkinen commentedon Jun 20, 2023
That's OK. I didn't first realize that there are course-level user stats stored in the cache. I am not against having user-specific course-level points cache. Your suggestion sounds good (both the original suggestion and the later).
Indeed, it is best to test the performance in practice to find the fastest solution.
markkuriekkinen commentedon Oct 19, 2023
#1231 fixed some parts of this issue. See the pull request description.