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

Emit multiple xAPI events for a multi-question submission #325

Closed

Conversation

pomegranited
Copy link
Contributor

@pomegranited pomegranited commented Jul 31, 2023

Description:

Updates the base event processor and tests to support transforming a single event into multiple events, and
modifies the server-side problem_check xAPI event transformer to transform multi-problem submissions into:

  • 1 event for the problem submission, plus
  • 1 event for each sub-problem

Resolves #219

JIRA: FAL-3430 (OpenCraft internal link)

Testing instructions:

This change should be covered by tests, but to test it manually:

  1. Install this branch on your Tutor LMS
  2. Login, and submit various responses to the Multiple Choice Questions block in the Demo course, or create your own.
  3. Watch multiple xAPI events flow through into Aspects from a single submission.

Merge checklist:

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

Post merge:

  • Create a tag
  • Check new version is pushed to PyPI after tag-triggered build is
    finished.

Author concerns:

  • Suggest reviewing this commit by commit to understand the reasoning.
  • This change only affects xAPI, not Caliper events.
  • Some minor changes to single question problem_check events occurred: added object.definition.name and interactionType.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Aug 3, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Aug 3, 2023

Thanks for the pull request, @pomegranited! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

Expected fixtures record the current behavior
Test was failing locally because test4.com is a real URL that responded
with a 200 when we posted events to it.

Not sure why it was succeeding in CI?
Updates the event processor change to allow for the possibility that
some event transformers may generate multiple events from a single
event, and these events need to be carried down the processor chain.
…mation

This reduces the side-effects of event transformation, which will allow
us to re-use base event transformation methods when generating multiple
events from a single source event.

* Removes the BaseTransformerMixin.transformed_event instance variable
  in favor of passing an event through to base_transform() to be
  modified and returned.
* Adds BaseTransformerMixin.get_object() so that caliper events don't
  need to reference self.transformed_event when updating the object data.
* Adds BaseTransformerMixin.get_extensions() so that caliper events can
  don't have to hack in their transformerVersion during
  BaseTransformerMixin.transform()
Single-question problem_check events still only produce one xAPI event.

Changes to the top-level multi-question problem_check event data:

* object.type changed from Activity to GroupActivity
* object.id shows the base problem usage_key
* object.definition.interaction_type is "other"

New events emitted for each child problem are identical the top-level event,
except for:

* object.type is Activity
* object.id shows the base problem usage_key including the child usage string
* object.definition.interaction_type is determined by the child problem response_type
* result.score is omitted -- only relevant to the parent problem
* result.response is provided, pulled from the child question submission
* result.success is provided, pulled from the child question submission

Related fixes to all problem_check events:

* object.definition.name now shows the problem display_name
* object.id now uses shows the problem usage_key
* result.score max and raw are now always provided if present in the
  source event (bug fix)
@pomegranited pomegranited force-pushed the jill/multi-question-xapi branch 4 times, most recently from 412de79 to 6ad3492 Compare August 7, 2023 10:40
@pomegranited pomegranited marked this pull request as ready for review August 7, 2023 10:40
@pomegranited
Copy link
Contributor Author

@bmtcril CC @Ian2012 @ziafazal This is ready for review :)

I think the coverage tests are failing because I hit the Actions too frequently when re-pushing my individual commits. I'll try that again tomorrow.

Copy link
Contributor

@bmtcril bmtcril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really good! Couple of things:

  1. I think this should be a major version bump as it's a pretty big breaking change for downstream data handling
  2. The coverage failures just happen in this repo pretty frequently, we just retry failed jobs when they come up
  3. When testing with the demo course I ran into this error when submitting the "Multiple Choice Questions" problem (single answer problems worked great):
2023-08-07 10:37:23 2023-08-07 14:37:23,344 ERROR 19 [eventtracking.backends.routing] [user None] [ip None] routing.py:146 - Unable to send edx event "problem_check" to backend: xapi
2023-08-07 10:37:23 Traceback (most recent call last):
2023-08-07 10:37:23   File "/openedx/venv/lib/python3.8/site-packages/eventtracking/backends/routing.py", line 137, in send_to_backends
2023-08-07 10:37:23     backend.send(event)
2023-08-07 10:37:23   File "/openedx/venv/lib/python3.8/site-packages/event_routing_backends/backends/events_router.py", line 158, in send
2023-08-07 10:37:23     event_routes = self.prepare_to_send([event])
2023-08-07 10:37:23   File "/openedx/venv/lib/python3.8/site-packages/event_routing_backends/backends/events_router.py", line 82, in prepare_to_send
2023-08-07 10:37:23     processed_events = self.process_event(event)
2023-08-07 10:37:23   File "/openedx/venv/lib/python3.8/site-packages/event_routing_backends/backends/events_router.py", line 191, in process_event
2023-08-07 10:37:23     events = processor(events)
2023-08-07 10:37:23   File "/openedx/venv/lib/python3.8/site-packages/event_routing_backends/processors/mixins/base_transformer_processor.py", line 37, in __call__
2023-08-07 10:37:23     transformed_event = self.transform_event(event)
2023-08-07 10:37:23   File "/openedx/venv/lib/python3.8/site-packages/event_routing_backends/processors/xapi/transformer_processor.py", line 48, in transform_event
2023-08-07 10:37:23     event_json = transformed_event.to_json()
2023-08-07 10:37:23 AttributeError: 'list' object has no attribute 'to_json'

* fixes "AttributeError: 'list' object has no attribute 'to_json'"
  during event processing for one-to-many problem_check events
* adds test for ^
* reverts unneeded change to test_caliper to reinstate 100% test coverage
@pomegranited
Copy link
Contributor Author

Thank you for testing this @bmtcril !

  1. I think this should be a major version bump as it's a pretty big breaking change for downstream data handling

Done: ea3d27d

  1. The coverage failures just happen in this repo pretty frequently, we just retry failed jobs when they come up

👍

  1. When testing with the demo course I ran into this error when submitting the "Multiple Choice Questions" problem (single answer problems worked great):

Ach sorry.. I missed that during a refactor, since the tests were passing.. 40f7422 covers that error and adds a test.

Copy link
Contributor

@bmtcril bmtcril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there, I think. What I see now is that the statements all get created with the same event id. I'd expect the child statements to each have their own id, but that the StatementRef would look up to the parent. As it stands, the child events get de-duplicated down to 1 event in the database because they have identical id/actor/object/verb/timestamps.

Screenshot 2023-08-08 at 2 20 23 PM

* stops mocking uuid5 during tests
  uuid5 generates the same UUID when provided with the same namespace + name,
  and so we can rely on this remaining the same in the expected fixture files.
* updates test data to use the actual uuid5s generated for the given input events.
* pulls event ID generation into get_event_id(),
  taking care not to modify how parent event IDs are generated.
* overrides get_event_id() for child events by using the child_id as
  part of the UUID namespace_key.
* updates tests to check that child events and their parent event use
  different event IDs, and the affected problem_check multiple-question
  test data
@pomegranited
Copy link
Contributor Author

@bmtcril Gotcha. I decided to do this in two separate commits:

  • 885f476 -- removes the mock uuid5 from the test wrapper so that our tests validate the actual generated event ID for all existing xAPI tests.
    This touches a lot of files, and I'm happy to move it into a separate PR if you'd prefer. But it was necessary for the next step, because the mocked uuid5 caused the parent event to have the same ID as its child events.
  • 9482075 -- makes child events use a different event ID from the parent.

@pomegranited
Copy link
Contributor Author

Hiya @bmtcril , when you get a chance -- did I fix the de-duplication issue you noted?

@bmtcril
Copy link
Contributor

bmtcril commented Aug 14, 2023

Hey there, I'll run again tomorrow. I've been flat out trying to get the asset import pr in shape

@pomegranited
Copy link
Contributor Author

No worries at all, thank you for your thorough testing @bmtcril !

@bmtcril
Copy link
Contributor

bmtcril commented Aug 15, 2023

Ran into some new issues here today, but haven't had time to sort them out. Ralph is failing insert on all three child events with:

2023-08-15 16:48:30 Code: 349. DB::Exception: Cannot convert NULL value to non-Nullable type: while converting source column org to destination column org: while executing 'FUNCTION _CAST(org :: 5, String :: 8) -> _CAST(org, String) String : 10': while pushing to view xapi.xapi_events_all_parsed_mv (778579dd-f27a-4ef7-a7d1-0a137ee42cba): While executing WaitForAsyncInsert. (CANNOT_INSERT_NULL_IN_ORDINARY_COLUMN) (version 23.3.4.17 (official build))

This is what the child event looks like in the db:

{"actor": {"account": {"homePage": "http://local.overhang.io", "name": "f7f6e171-9a01-480a-b330-7132f02f292e"}, "objectType": "Agent"}, "id": "324cbcfa-6e40-593b-8241-1efc604a03e0", "object": {"id": "http://local.overhang.io/xblock/block-v1:edX+DemoX+Demo_Course+type@problem+block@a0effb954cca4759994f1ac9e9434bf4_4_1", "objectType": "Activity", "definition": {"description": {"en-US": ""}, "type": "http://adlnet.gov/expapi/activities/cmi.interaction", "interactionType": "choice", "extensions": {"http://id.tincanapi.com/extension/attempt-id": 2}}}, "verb": {"id": "https://w3id.org/xapi/acrossx/verbs/evaluated", "display": {"en": "evaluated"}}, "version": "1.0.3", "timestamp": "2023-08-15T20:48:29.026858+00:00", "context": {"contextActivities": {"parent": [{"id": "http://local.overhang.io/xblock/block-v1:edX+DemoX+Demo_Course+type@problem+block@a0effb954cca4759994f1ac9e9434bf4", "objectType": "Activity", "definition": {"description": {"en-US": ""}, "type": "http://adlnet.gov/expapi/activities/cmi.interaction", "interactionType": "other", "extensions": {"http://id.tincanapi.com/extension/attempt-id": 2}}}], "grouping": [{"id": "http://local.overhang.io/course/course-v1:edX+DemoX+Demo_Course", "objectType": "Activity", "definition": {"name": {"en-US": "Demonstration Course"}, "type": "http://adlnet.gov/expapi/activities/course"}}]}, "statement": {"objectType": "StatementRef", "id": "1eef567e-8ded-52cc-b7de-1d1efd432708"}, "extensions": {"https://w3id.org/xapi/openedx/extension/transformer-version": "[email protected]", "https://w3id.org/xapi/openedx/extensions/session-id": "7fd5f7f471a9a2a95709accad1467f80"}}, "result": {"success": true, "response": "['a piano', 'a guitar']"}}

@bmtcril
Copy link
Contributor

bmtcril commented Aug 15, 2023

Looks like it's trying to get the course id and failing since it's now in "grouping":

"grouping": [{"id": "http://local.overhang.io/course/course-v1:edX+DemoX+Demo_Course", "objectType": "Activity", "definition": {"name": {"en-US": "Demonstration Course"}, "type": "http://adlnet.gov/expapi/activities/course"}}]}

I think we'll need a migration to update the sql, it's trying to do:

       if(JSON_VALUE(event_str,
                     '$.context.contextActivities.parent[0].definition.type') =
          'http://adlnet.gov/expapi/activities/course',
          JSON_VALUE(event_str, '$.context.contextActivities.parent[0].id'),
          JSON_VALUE(event_str, '$.object.id'))                   AS course_id,

Copy link
Contributor

@bmtcril bmtcril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is working for me from the xAPI side, but we need to update Aspects before we can upgrade do a new version!

@Ian2012
Copy link
Contributor

Ian2012 commented Sep 12, 2023

@bmtcril what is needed to include this change in Aspects?

@bmtcril
Copy link
Contributor

bmtcril commented Sep 13, 2023

@Ian2012 this will need a big rebase and testing again. We need to add a migration to xapi_events_all_parsed_mv to look for the course id in the new location. I think we'll need some changes to problem_events_mv to roll these up into whatever the logical structure is there, but I'm not sure what off hand.

@bmtcril bmtcril mentioned this pull request Sep 25, 2023
@bmtcril
Copy link
Contributor

bmtcril commented Sep 25, 2023

I'm working to get this over the line in #352

@bmtcril bmtcril closed this Sep 25, 2023
@openedx-webhooks
Copy link

@pomegranited Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

1 similar comment
@openedx-webhooks
Copy link

@pomegranited Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@pomegranited pomegranited deleted the jill/multi-question-xapi branch June 14, 2024 03:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Multi-question submission emits only a single xAPI statement
4 participants