feat: auto import course structure on course publish #43
Conversation
| @@ -85,3 +85,17 @@ cairn-postgresql: | |||
| depends_on: | |||
| - permissions | |||
| {% endif %} | |||
| cairn-watchcourses: | |||
There was a problem hiding this comment.
What are the additional CPU/memory resources that are needed by this container? (see docker stats) Given that it's a very thin wrapper, I expect that the requirements are low. But if they are not, then we'll have to gatekeep this service behind a feature flag.
There was a problem hiding this comment.
My environment is the following:
- Docker 4.28.0
- Tutor 18.1.1
- Plugins enabled: Cairn
My machine specs are the following:
- MacOS Sonoma
- Macbook Pro M1
Resources utilized:
When the container is sitting idle, listening to requests:
- CPU: 0.00 - 0.03%
- Memory: ~58 MiB
When the container is executing the importcoursedata script:
- CPU: 100% (on MacOS, this usually signifies one core is being completely utilized)
- Memory: 250 MiB - 300 MiB
Then it returns back to idle resources once it is completed.
These results would definitely vary on a linux based machine.
| && uvicorn --app-dir /openedx/scripts/ main:app --host 0.0.0.0 --port {{ CAIRN_WATCHCOURSES_PORT }}" | ||
| restart: unless-stopped | ||
| environment: | ||
| SETTINGS: ${TUTOR_EDX_PLATFORM_SETTINGS:-tutor.production} |
There was a problem hiding this comment.
Can we avoid defining this environment variable here? After all, the fastapi process only needs it for its subprocess, not its main process. I suggest removing it from the service declaration and use the env option in subprocess.call(... env=...) (docs).
There was a problem hiding this comment.
It just works without defining this environment variable. Checking the value of os.environ, it reveals that the container is already using the production settings.
This is what I found in os.environ.
'DJANGO_SETTINGS_MODULE': 'lms.envs.tutor.production'
There was a problem hiding this comment.
This is going to work in production because the container inherits the settings from the image definition. But you should make sure that it's correct in dev and in kubernetes.
| inputs = ["course_published"] | ||
| batch.timeout_secs = 300 | ||
| batch.max_events = 10 | ||
| uri = "http://cairn-watchcourses:9282/import_course/" |
There was a problem hiding this comment.
The name does not seem very adequate anymore, now that we can pass several course IDs at once. Maybe /courses/published?
There was a problem hiding this comment.
That sounds much better, I'll update it.
|
|
||
| data = await request.json() | ||
| if not isinstance(data, list) or 'course_id' not in data[0]: | ||
| return web.json_response({"error":"Value course_id is required."}, status=400) |
There was a problem hiding this comment.
Let's return two different errors based on the data format:
if not isinstance(data, list):
return web.json_response({"error": f"Incorrect data format. Expected list, got {data.__class__}."}, status=400)
course_ids = []
for course in data:
course_id = data.get("course_id")
if not isinstance(course_id, str):
return web.json_response({"error": f"Incorrect course_id format: expected str, got {course_id.__class__}."}, status=400)
There was a problem hiding this comment.
Thank you, this looks great but according to this, even if one course_id is invalid, we return an error. Shouldn't we still try to run import for the valid course_ids instead?
| # Verify course_id is a valid course_id | ||
| try: | ||
| CourseLocator.from_string(course_id) | ||
| except: |
There was a problem hiding this comment.
We should almost never silence an arbitrary error. At the very least, we should log.exception(e).
|
|
||
| # If none of the course_ids are valid, return an error | ||
| if not course_ids: | ||
| return web.json_response({"error": f"Invalid course_id"}, status=400) |
There was a problem hiding this comment.
Why "invalid course_id"? The message should explain that we expect course_id as arguments, and we didn't get any.
There was a problem hiding this comment.
This was to signify that the format of the course_id itself is invalid. But I will update this to a better sounding message.
| && uvicorn --app-dir /openedx/scripts/ main:app --host 0.0.0.0 --port {{ CAIRN_WATCHCOURSES_PORT }}" | ||
| restart: unless-stopped | ||
| environment: | ||
| SETTINGS: ${TUTOR_EDX_PLATFORM_SETTINGS:-tutor.production} |
There was a problem hiding this comment.
This is going to work in production because the container inherits the settings from the image definition. But you should make sure that it's correct in dev and in kubernetes.
| # Configure logging | ||
| logging.basicConfig(level=logging.INFO, | ||
| format='%(asctime)s - %(name)s - %(levelname)s - %(message)s') | ||
|
|
||
| # Get a logger instance |
There was a problem hiding this comment.
no need for the logging comments here, the code is self-explanatory.
| @@ -0,0 +1,45 @@ | |||
| from aiohttp import web | |||
There was a problem hiding this comment.
- nit: a file docstring will be gelpful to understand the context.
- Please check that imports are ordered correctly (first party \n third party \n local code)
There was a problem hiding this comment.
I've sorted the imports and added a file docstring as well.
Danyal-Faheem
deleted the
danyalfaheem/import-course-data-on-course-publish
branch
Changes
Updating course overview for <course_id>logscairn-watchcoursesthat listens for incoming events from vectorimportcoursedatascript fromcairn-watchcoursesfor the specific course whenever a course is publishedCaveats