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

feat: mark optional completions #187

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DanielVZ96
Copy link
Member

@DanielVZ96 DanielVZ96 commented Feb 17, 2024

Description: Adds support for optional completions by marking completions as optional, and adding them to the mean accordingly.

JIRA: https://tasks.opencraft.com/browse/BB-8586

Dependencies: TODO: add edx-platform PR

Installation instructions: List any non-trivial installation
instructions.

Testing instructions:

1.Install the app from this branch

  • Modify the devstack docker-compose to install this branch locally: 'source /edx/app/edxapp/edxapp_env && pip install -e /edx/src/openedx-completion-aggregator && while true; do python /edx/app/edxapp/edx-platform/manage.py lms runserver 0.0.0.0:18000 --settings devstack_docker; sleep 2; done'
  • Restart the containers
  • Add the completion_aggregator app to the Advanced Module List inside Advanced Settings in CMS
  1. Collect the aggregations running the following commands in the lms container:
  • ./manage.py lms reaggregate_course
  • ./manage.py lms run_aggregator_service
  1. Request the aggregations at this url http://localhost:18000/completion-aggregator/v1/course/course-v1:edX+DemoX+Demo_Course/?requested_fields=chapter,vertical

Reviewers:

  • tag reviewer

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed
  • PR author is listed in AUTHORS

Post merge:

  • Create a tag
  • Check new version is pushed to PyPI after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

Author concerns: Ideally optional completions should be included in an additional field, but due to time constraints I wasn't able to implement that here.

Private-ref: BB-8586

@DanielVZ96 DanielVZ96 force-pushed the dvz/optional-completions branch 3 times, most recently from a18d041 to 582c238 Compare February 17, 2024 20:40
@DanielVZ96 DanielVZ96 marked this pull request as ready for review February 17, 2024 20:44
@DanielVZ96
Copy link
Member Author

@Agrendalath codecov actually improved in most of files, except one where it was lowered due to an increase of lines it semes: https://app.codecov.io/gh/open-craft/openedx-completion-aggregator/pull/187

@Agrendalath
Copy link
Member

@DanielVZ96, just one initial question: what are this change's performance implications? The crucial aspect of the completion aggregator is the ability to process thousands of completions per second. Can we make a quick performance comparison - before and after this change?

Side note:

'source /edx/app/edxapp/edxapp_env && pip install -e /edx/src/openedx-completion-aggregator && while true; do python /edx/app/edxapp/edx-platform/manage.py lms runserver 0.0.0.0:18000 --settings devstack_docker; sleep 2; done'

You can add the following to options.local.mk in your devstack directory:

# Restart both containers
edx-restart: lms-restart studio-restart

# This allows you to install an XBlock in both LMS and Studio using
# make install-xblock XBLOCK=problem-builder
install-xblock:
        for c in lms studio ; do \
                docker exec -it edx.$(COMPOSE_PROJECT_NAME).$$c bash -c \
                'cd /edx/src/$(XBLOCK) && /edx/app/edxapp/venvs/edxapp/bin/pip install -e .' ;   \
        done
        make edx-restart

@DanielVZ96
Copy link
Member Author

@DanielVZ96, just one initial question: what are this change's performance implications? The crucial aspect of the completion aggregator is the ability to process thousands of completions per second. Can we make a quick performance comparison - before and after this change?

Side note:

'source /edx/app/edxapp/edxapp_env && pip install -e /edx/src/openedx-completion-aggregator && while true; do python /edx/app/edxapp/edx-platform/manage.py lms runserver 0.0.0.0:18000 --settings devstack_docker; sleep 2; done'

You can add the following to options.local.mk in your devstack directory:

# Restart both containers
edx-restart: lms-restart studio-restart

# This allows you to install an XBlock in both LMS and Studio using
# make install-xblock XBLOCK=problem-builder
install-xblock:
        for c in lms studio ; do \
                docker exec -it edx.$(COMPOSE_PROJECT_NAME).$$c bash -c \
                'cd /edx/src/$(XBLOCK) && /edx/app/edxapp/venvs/edxapp/bin/pip install -e .' ;   \
        done
        make edx-restart

will test performance tomorrow

@DanielVZ96
Copy link
Member Author

@DanielVZ96, just one initial question: what are this change's performance implications? The crucial aspect of the completion aggregator is the ability to process thousands of completions per second. Can we make a quick performance comparison - before and after this change?

Side note:

'source /edx/app/edxapp/edxapp_env && pip install -e /edx/src/openedx-completion-aggregator && while true; do python /edx/app/edxapp/edx-platform/manage.py lms runserver 0.0.0.0:18000 --settings devstack_docker; sleep 2; done'

You can add the following to options.local.mk in your devstack directory:

# Restart both containers
edx-restart: lms-restart studio-restart

# This allows you to install an XBlock in both LMS and Studio using
# make install-xblock XBLOCK=problem-builder
install-xblock:
        for c in lms studio ; do \
                docker exec -it edx.$(COMPOSE_PROJECT_NAME).$$c bash -c \
                'cd /edx/src/$(XBLOCK) && /edx/app/edxapp/venvs/edxapp/bin/pip install -e .' ;   \
        done
        make edx-restart

Here's some timings:

  • With changes:
    image
    image

  • Without changes:
    image
    image
    image

So in both:

  • System CPU time varies from 1 ms to 20 ms
  • Request time incremented from 168 ms to 458 ms

Based on this I can't say there's a clear performance penalty. Also as a side-note: with my current courses it's having to generate 233 completion aggregations.

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