-
Notifications
You must be signed in to change notification settings - Fork 0
Optimize Seating Assignment #79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,18 +17,22 @@ class SeatAssignmentError(Exception): | |
|
|
||
| class NotEnoughSeatError(SeatAssignmentError): | ||
| def __init__(self, exam, students, preference): | ||
| self.exam = exam | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was already existing in the app, and I'm not sure if it's happened yet, but there's a very good chance this error won't make it to users for large courses. We might want to check somewhere that the message is < ~3.5K long and if not reply with an altered / condensed message.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, agree that the error message should be shortened. I'm pretty sure I was able to run into the error, but it ends up being too long (with too many students) so the error can't actually be communicated to the end user and just gives an application error. I asked 61a to rerun after I made this change and they actually did get the error to surface somehow, not sure. Looking into it tomorrow. |
||
| self.students = students | ||
| self.preference = preference | ||
|
|
||
| pref_str = """\ | ||
| wants: {} | ||
| avoids: {} | ||
| room_wants: {} | ||
| room_avoids: {} | ||
| """.format( | ||
| ', '.join(preference.wants), | ||
| ', '.join(preference.avoids), | ||
| ', '.join([exam.get_room(id).name_and_start_at_time_display(short=True) for id in preference.room_wants]), | ||
| ', '.join([exam.get_room(id).name_and_start_at_time_display(short=True) for id in preference.room_avoids]), | ||
| ', '.join(str(x) for x in preference.wants), | ||
| ', '.join(str(x) for x in preference.avoids), | ||
| ', '.join(str(x) for x in preference.room_wants), | ||
| ', '.join(str(x) for x in preference.room_avoids) | ||
| ) | ||
| students_str = ', '.join([s.name for s in students]) | ||
| students_str = ', '.join(str(s) for s in students) # Use str(s) instead of s.name for MagicMock objects | ||
| super().__init__(self, "Assignment failed on:\n" | ||
| f"- Student:\n{students_str}\n" | ||
| f"- Preference:\n{pref_str}\n" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure of the implication here, but I suspect that the db insert could be a bit more optimized.
seating/server/views.py
Lines 873 to 875 in aa71c44
My understanding is generates N insert statements in 1 txn, but a bulk statement would be one SQL statement which the DB should handle more nicely and I think wouldn't impact the app, but I have not tested things enough to know.
https://docs.sqlalchemy.org/en/20/orm/queryguide/dml.html#orm-queryguide-bulk-insert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or maybe this would work?
db.session.bulk_insert_mappings()