Skip to content
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

[task_dispensers] Task settings migration #964

Closed
wants to merge 2 commits into from

Conversation

anthonygego
Copy link
Member

This PR ends the long awaited process of task settings migration to task dispensers, first step to ease task sharing among courses.

As discussed with @nrybowski, the online but manual way was preferred to trigger the task file update. Therefore, a migration option is added in top of the task list page in case the current task dispenser detects legacy settings in the task files. When the upgrade is asked, the current task dispenser will import the task settings into its own data structure and then the fields imported will be removed from task file descriptors.

This is implemented by adding two function definitions to task dispensers : has_legacy_tasks and import_legacy_tasks as well as a legacy_fields attribute to workaround the fact task dispensers have no access to the descriptors to modify the files themselves. A default newly defined task dispenser will inherit these definitions but they will have no effect on the tasks. They are purposely not defined as abstract methods then.

@anthonygego anthonygego changed the title Task settings migration to task dispensers [task_dispensers] Task settings migration Aug 9, 2023
@nrybowski
Copy link
Member

Tested on 61ead04 with the LEPL1402.

The frontend fails while reading the course.yaml :

inginious-frontend-1      | 2023-10-11 12:15:46,690 - inginious.course_factory - INFO - Caching course LEPL1402
inginious-frontend-1      | 2023-10-11 12:15:46,777 - inginious.course.LEPL1402 - WARNING - Cannot open course
inginious-frontend-1      | Traceback (most recent call last):
inginious-frontend-1      |   File "/inginious/inginious/frontend/courses.py", line 92, in __init__
inginious-frontend-1      |     self._task_dispenser = task_dispenser_class(lambda: self._task_factory.get_all_tasks(self), self._content.get("dispenser_data", ''), database, self.get_id())
inginious-frontend-1      |   File "/inginious/inginious/frontend/task_dispensers/toc.py", line 27, in __init__
inginious-frontend-1      |     raise Exception("Invalid dispenser data structure")
inginious-frontend-1      | Exception: Invalid dispenser data structure
inginious-frontend-1      | 
inginious-frontend-1      | During handling of the above exception, another exception occurred:
inginious-frontend-1      | 
inginious-frontend-1      | Traceback (most recent call last):
inginious-frontend-1      |   File "/inginious/inginious/frontend/course_factory.py", line 126, in get_all_courses
inginious-frontend-1      |     output[courseid] = self.get_course(courseid)
inginious-frontend-1      |   File "/inginious/inginious/frontend/course_factory.py", line 52, in get_course
inginious-frontend-1      |     self._update_cache(courseid)
inginious-frontend-1      |   File "/inginious/inginious/frontend/course_factory.py", line 234, in _update_cache
inginious-frontend-1      |     Course(courseid, course_descriptor, self.get_course_fs(courseid), self._task_factory, self._plugin_manager, self._task_dispensers, self._database),
inginious-frontend-1      |   File "/inginious/inginious/frontend/courses.py", line 94, in __init__
inginious-frontend-1      |     raise Exception("Course has an invalid YAML spec: " + self.get_id())
inginious-frontend-1      | Exception: Course has an invalid YAML spec: LEPL1402

The actual raised exception is Invalid dispenser data structure.

@nrybowski
Copy link
Member

Applying grouped_actions and then tasksets on top of this branch seems to solve the issue.
I suppose that the tasksets patches convert old courses in a correctly formatted taskset and course.

@nrybowski nrybowski added this to the v0.8.8 milestone Oct 24, 2023
@anthonygego
Copy link
Member Author

Applying grouped_actions and then tasksets on top of this branch seems to solve the issue. I suppose that the tasksets patches convert old courses in a correctly formatted taskset and course.

I'll close this one and focus on #982

@anthonygego anthonygego deleted the task_dispenser_migr branch November 13, 2023 10:45
@nrybowski nrybowski mentioned this pull request Jan 10, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants