feat: [FC-0074] add support for annotated python dicts as avro map type #433

mariajgrimaldi · 2024-12-12T18:14:30Z

Description

This PR supports Python dict types mapped to Avro Map type for avro schema generation, aiming to add support to a broader list of events payloads. This was previously attempted by mapping Python dicts -> records but this approach considers Python dicts -> maps to avoid conflicts with data attributes -> record mapping, also by using maps we avoid this kind of errors when we don't know the content of dictionaries:

This PR also refactors forum-related events so they can be sent through the event bus. Because of backward compatibility, those changes should be studied in a different PR but are here for testing simplicity.

Supporting information

This PR addresses #428 (comment)

Testing instructions

To test with the event bus:

Install this branch into your environment
Install an event bus backend like the redis implementation in the LMS
Create a tutor plugin with this configuration so the event is produced to redis:

from tutor import hooks

redis_config = [
    "EVENT_BUS_PRODUCER = 'edx_event_bus_redis.create_producer'",
    "EVENT_BUS_REDIS_CONNECTION_URL = 'redis://@redis:6379/'",
    "EVENT_BUS_TOPIC_PREFIX = 'dev'",
    "EVENT_BUS_CONSUMER = 'edx_event_bus_redis.RedisEventConsumer'",
]

event_bus_config = """
EVENT_BUS_PRODUCER_CONFIG = {
    'org.openedx.learning.forum.thread.created.v1': {
         'forum-thread-created': {'event_key_field': 'thread.id', 'enabled': True},
     },
     'org.openedx.learning.forum.thread.response.created.v1': {
         'forum-thread-response-created': {'event_key_field': 'thread.id', 'enabled': True},
     },
     'org.openedx.learning.forum.thread.response.comment.created.v1': {
         'forum-thread-response-comment-created': {'event_key_field': 'thread.id', 'enabled': True},
     },
}
"""

hooks.Filters.ENV_PATCHES.add_item(
    (
        "openedx-common-settings", event_bus_config
    )
)

You should see a log similar to this in the LMS:

Deadline

None

Other information

This was previously attempted here: but using record type instead of the map type: #232

Checklists

Check off if complete or not applicable:

Merge Checklist:

Post Merge:

Create a tag
Create a release on GitHub
Check new version is pushed to PyPI after tag-triggered build is
finished.
Delete working branch (if not needed anymore)
Upgrade the package in the Open edX platform requirements (if applicable)

openedx-webhooks · 2024-12-12T18:14:35Z

Thanks for the pull request, @mariajgrimaldi!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/hooks-extension-framework. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

Ian2012 · 2024-12-12T18:41:19Z

openedx_events/event_bus/avro/tests/test_avro.py

+        dict[str, str]: {'key': 'value'},
+        dict[str, int]: {'key': 1},
+        dict[str, float]: {'key': 1.0},
+        dict[str, bool]: {'key': True},


what about union types:

dict[str, Union[str, int]]: ...,

I added more test cases and this covers even more than I initially thought: a701f78. Thanks for the suggestion!

Ian2012 · 2024-12-13T19:32:59Z

openedx_events/event_bus/avro/schema.py

+        # returns types of dict contents
+        # if data_type == Dict[str, int], arg_data_type = (str, int)


nit: remove code comments

I don't think that's commented code but an explanation of what arg_data_type is

I had the same confusion. What about a change like the following? Note that this was copied from above, so I'd make the same fix there as well.

Suggested change

# returns types of dict contents

# if data_type == Dict[str, int], arg_data_type = (str, int)

# Returns types of dict contents.

# Example: if data_type == Dict[str, int], arg_data_type = (str, int)

Will do. Thanks for the suggestion!

openedx_events/event_bus/avro/schema.py

openedx_events/event_bus/avro/serializer.py

Ian2012 · 2024-12-13T19:36:37Z

openedx_events/learning/signals.py

@@ -297,7 +297,7 @@
 # .. event_data: DiscussionThreadData
 # .. event_warning: This event is currently incompatible with the event bus, list/dict cannot be serialized yet
 FORUM_THREAD_CREATED = OpenEdxPublicSignal(
-    event_type="org.openedx.learning.thread.created.v1",
+    event_type="org.openedx.learning.forum.thread.created.v1",


Does this need to be changed?
Should this emit a FORUM_THREAD_CREATED_V2 signal?

I imagine this will break existing code

Oh yes, as I mentioned in the cover letter those changes are there for testing. That should be addressed in a follow up PR.

Ian2012

Some nits and concerns around changing existing code and some code cleanup

felipemontoya

The tradeoff of supporting dicts only when annotated seems completely fair to me.

I agree that the refactoring of the forum events should go in a separate PR. Do you think this needs that we bump the versioning of the events or are they backwards compatible?

mariajgrimaldi · 2024-12-20T11:26:51Z

I agree that the refactoring of the forum events should go in a separate PR. Do you think this needs that we bump the versioning of the events or are they backwards compatible?

I do think it's backwards compatible. We'd be affecting the event bus support, which we didn't have previously, so I think we're okay.

bmtcril

This looks great to me, but I haven't been in this code much so I'll refrain from thumbing it.

mariajgrimaldi · 2024-12-26T09:36:29Z

I'm tagging @robrap @timmc-edx for a review since they're more familiar with the event bus code. Can you help us out here? Do you think this looks reasonable?

timmc-edx · 2024-12-26T19:38:57Z

We're currently on break but should be able to review after Jan 6.

robrap

@mariajgrimaldi: I gave some comments, but I feel like I don't know enough to really understand the change (see other comments for details).

robrap · 2025-01-07T21:18:50Z

openedx_events/event_bus/avro/schema.py

+        # returns types of dict contents
+        # if data_type == Dict[str, int], arg_data_type = (str, int)


I had the same confusion. What about a change like the following? Note that this was copied from above, so I'd make the same fix there as well.

Suggested change

# returns types of dict contents

# if data_type == Dict[str, int], arg_data_type = (str, int)

# Returns types of dict contents.

# Example: if data_type == Dict[str, int], arg_data_type = (str, int)

robrap · 2025-01-07T21:23:56Z

openedx_events/event_bus/avro/schema.py

@@ -63,7 +63,7 @@ def _create_avro_field_definition(data_key, data_type, previously_seen_types,
        field["type"] = field_type
    # Case 2: data_type is a simple type that can be converted directly to an Avro type
    elif data_type in PYTHON_TYPE_TO_AVRO_MAPPING:
-        if PYTHON_TYPE_TO_AVRO_MAPPING[data_type] in ["record", "array"]:
+        if PYTHON_TYPE_TO_AVRO_MAPPING[data_type] in ["map", "array"]:


This is a breaking change, right? Were dicts only introduced and used with the forums events, which you plan to break anyway? You don't have a changelog entry or a major version change (yet), but is that your plan?

As far as I understand, dictionaries were never fully supported. Besides this list with known unserializable events, which contain dictionaries in their payloads, we can confirm this by generating a schema for an event using dictionaries. Let's take this event as an example:

# openedx_events/learning/signals.py@main MY_EVENT = OpenEdxPublicSignal( event_type="org.openedx.learning.my_event.v1", data={ "my_data": MyEventData, } ) # openedx_events/learning/data.py@main @attr.s(frozen=True) class MyEventData: event_type = attr.ib(type=str) event_data = attr.ib(type=dict[str, str])

Which immediately raises a serialization error:

openedx-events/openedx_events/event_bus/avro/schema.py", line 107, in _create_avro_field_definition raise TypeError( TypeError: Data type dict[str, str] is not supported. The data type needs to either be one of the types in PYTHON_TYPE_TO_AVRO_MAPPING, an attrs decorated class, or one of the types defined in custom_type_to_avro_type.

Because dicts were not a type supported in event_bus/avro/schema.py::_create_avro_field_definition before this PR. So no events with type dicts were ever sent through the event bus. Forum events were not supported either since they were listed in the known unserialiazable events, but I included them here as an example and will remove them shortly.

To maintain backward compatibility, I'll include the map type as an additional type instead of changing "record" to "map".

Thank you @mariajgrimaldi.

It sounds like changing from "record" to "map" is not a breaking change for dicts if they aren't yet in use, and is really just a fix.

Whether or not dropping "record" is a breaking change elsewhere is still a question. No tests broke when you had dropped it. Does that mean it isn't used, or that we are missing test coverage.

Highly related, why do I not see the /schemas introduced in https://github.com/openedx/openedx-events/pull/225/files for unit testing? Is that part of the missing coverage?

UPDATE: If we are missing test coverage, it would be great to get that back. Or, if this is a non-breaking change, then you should drop "record" to clean up the code.

I added some tests to solve the missing coverage. However, I still wonder if we're breaking something by replacing "record" with "map". I also checked the /schemas added in https://github.com/openedx/openedx-events/pull/225/files and they are still to be in the repository. Could you clarify which schemas are missing?

Oops. I now see the schemas. In theory, if our tests are working as planned, these should be snapshots of all the schemas and if there is no change detected, all would be good.

You say you fear dropping record is a breaking change. Is it possible to create a breaking test?

From my understanding, changing "record" to "map" should not be a breaking change since dicts were not supported before this PR. By testing the code with dicts without any support, I can infer that L66 was only used to warn developers that dicts (mapped to avro records, which is a type we can't use because it clashes with the data attributes mapping) without annotations were not allowed, but either dicts with annotations since they were not supported. In any case, I added a test case that checks that the schema generated for dicts should map to map for future reference.

Although I said I wondered whether this was a breaking change given my findings and considering what you mentioned before, I think we should trust the repo's coverage.

robrap · 2025-01-07T22:34:24Z

...events/event_bus/avro/tests/schemas/org+openedx+learning+forum+thread+created+v1_schema.avsc

I'm curious why there are no old test schemas? I would have expected a change from old (deleted files) to new (these files). They would have shown before/after.

My head isn't in this world enough to understand the difference between record and map, and why it is important to move from one to the other.

Sorry for all the confusion with these events. Forum events were never supported by the events because they included dict types in their payloads, that's why there is not an old schema. As for why using maps instead of records, it's mainly because of this:

This PR supports Python dict types mapped to Avro Map type for avro schema generation, aiming to add support to a broader list of events payloads. This was previously attempted by mapping Python dicts -> records but this approach considers Python dicts -> maps to avoid conflicts with data attributes -> record mapping, also by using maps we avoid this kind of errors when we don't know the content of dictionaries:

Thank you. This does clarify for forum events.

timmc-edx · 2025-01-07T23:52:29Z

openedx_events/event_bus/avro/deserializer.py

+        if arg_data_type[1] in SIMPLE_PYTHON_TYPE_TO_AVRO_MAPPING:
+            return data


Do we also need to check whether the dict key type is a simple Python type?

I added the check to study the behavior, and I got a few errors because the keys are unhashable by nature. Therefore, only basic Python types were allowed. Do you think we need an explicit check for this?

Can you say more about keys being unhashable? What's a situation where that happens?

When using types as keys that are not simple python types, let's say SIGNAL = create_simple_signal({"dict_input": Dict[ComplexAttrs, int]}) where ComplexAttrs is unhashable I get this error:

FAILED openedx_events/event_bus/avro/tests/test_deserializer.py::TestAvroSignalDeserializerCache::test_deserialization_of_dicts_with_keys_of_complex_types_fails - TypeError: unhashable type: 'ComplexAttrs'

But keys of type CourseKey work. So, as you suggested, I'm going to add the additional check.

robrap

@mariajgrimaldi: 👍. Please follow your normal process for landing this. I don't see any blockers. The only reason I am not approving is because I don't plan to review all the final code (tests) in depth. Thanks for checking in on this.

Also, just a reminder to update version and changelog at some point.

bmtcril

Based on the existing comments and my tests I think this is good and non-breaking. Just needs the usual version bump stuff. 👍

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 12, 2024

mariajgrimaldi mentioned this pull request Dec 12, 2024

docs: [FC-0074] add how-to add event bus support to an Open edX Event #428

Merged

Ian2012 reviewed Dec 12, 2024

View reviewed changes

mariajgrimaldi changed the title ~~Mjg/add support for dicts~~ feat: add support for annotated python dicts as avro map type Dec 13, 2024

Ian2012 and others added 2 commits December 13, 2024 17:22

feat: add support for Avro dict

7825196

refactor: add support for dicts as avro maps instead of record

9aaa04e

mariajgrimaldi force-pushed the MJG/add-support-for-dicts branch from e71efcc to acdcaa9 Compare December 13, 2024 16:23

mariajgrimaldi marked this pull request as ready for review December 13, 2024 18:42

mariajgrimaldi requested a review from a team as a code owner December 13, 2024 18:42

Ian2012 reviewed Dec 13, 2024

View reviewed changes

openedx_events/event_bus/avro/schema.py Show resolved Hide resolved

Ian2012 reviewed Dec 13, 2024

View reviewed changes

openedx_events/event_bus/avro/schema.py Show resolved Hide resolved

Ian2012 reviewed Dec 13, 2024

View reviewed changes

openedx_events/event_bus/avro/serializer.py Outdated Show resolved Hide resolved

Ian2012 reviewed Dec 13, 2024

View reviewed changes

Ian2012 requested changes Dec 13, 2024

View reviewed changes

mariajgrimaldi requested a review from bmtcril December 13, 2024 19:38

Ian2012 approved these changes Dec 13, 2024

View reviewed changes

mariajgrimaldi requested a review from felipemontoya December 16, 2024 09:42

felipemontoya reviewed Dec 20, 2024

View reviewed changes

mariajgrimaldi mentioned this pull request Dec 20, 2024

feat: add event bus support to forum events V2 #434

Closed

15 tasks

mariajgrimaldi changed the title ~~feat: add support for annotated python dicts as avro map type~~ feat: [FC-0074] add support for annotated python dicts as avro map type Dec 20, 2024

mariajgrimaldi added the FC Relates to an Axim Funded Contribution project label Dec 20, 2024

bmtcril reviewed Dec 20, 2024

View reviewed changes

mariajgrimaldi mentioned this pull request Dec 26, 2024

Add support for complex types in dictionaries and lists #441

Open

1 task

robrap reviewed Jan 7, 2025

View reviewed changes

timmc-edx reviewed Jan 7, 2025

View reviewed changes

mariajgrimaldi added 11 commits January 8, 2025 11:05

test: add more test cases for serializing dicts

743bcc4

refactor: address quality issues from latest commits

f4aa824

refactor: address PR reviews

f7beb63

fix: address quality issues

482c8d9

test: add deserializer tests for dicts

7d9b8b8

test: add tests for deserialization errors

c76b3d1

test: add test cases for schema generation with dict types

ec1fdec

fix: address quality issues

2d225b9

refactor: rewrite comment with suggestion

2839ae7

refactor: include map type instead of replacing record

4389cfb

refactor: drop changes in forum events

8f67ea2

mariajgrimaldi force-pushed the MJG/add-support-for-dicts branch from f86464a to 8f67ea2 Compare January 8, 2025 10:10

mariajgrimaldi added 4 commits January 8, 2025 11:11

refactor: drop changes in forum events

9ee35f2

refactor: address PR reviews

a32ac76

test: add missing test for de-serializing complex types

2980a93

refactor: check for keys and values simple types while de-serializing

7eabdaa

mariajgrimaldi requested review from bmtcril, robrap, felipemontoya, timmc-edx and Ian2012 January 13, 2025 14:04

robrap reviewed Jan 13, 2025

View reviewed changes

bmtcril approved these changes Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [FC-0074] add support for annotated python dicts as avro map type #433

feat: [FC-0074] add support for annotated python dicts as avro map type #433

mariajgrimaldi commented Dec 12, 2024 •

edited

Loading

openedx-webhooks commented Dec 12, 2024 •

edited

Loading

Ian2012 Dec 12, 2024

mariajgrimaldi Dec 13, 2024

Ian2012 Dec 13, 2024

mariajgrimaldi Dec 16, 2024

robrap Jan 7, 2025

mariajgrimaldi Jan 8, 2025

Ian2012 Dec 13, 2024

mariajgrimaldi Dec 13, 2024

Ian2012 left a comment

felipemontoya left a comment

mariajgrimaldi commented Dec 20, 2024

bmtcril left a comment

mariajgrimaldi commented Dec 26, 2024

timmc-edx commented Dec 26, 2024

robrap left a comment

robrap Jan 7, 2025

robrap Jan 7, 2025

mariajgrimaldi Jan 8, 2025

robrap Jan 8, 2025 •

edited

Loading

mariajgrimaldi Jan 10, 2025

robrap Jan 10, 2025

mariajgrimaldi Jan 13, 2025 •

edited

Loading

robrap Jan 7, 2025

mariajgrimaldi Jan 8, 2025

robrap Jan 8, 2025

timmc-edx Jan 7, 2025

mariajgrimaldi Jan 8, 2025

timmc-edx Jan 8, 2025

mariajgrimaldi Jan 10, 2025 •

edited

Loading

robrap left a comment

bmtcril left a comment

		# returns types of dict contents
		# if data_type == Dict[str, int], arg_data_type = (str, int)

		if arg_data_type[1] in SIMPLE_PYTHON_TYPE_TO_AVRO_MAPPING:
		return data

feat: [FC-0074] add support for annotated python dicts as avro map type #433

Are you sure you want to change the base?

feat: [FC-0074] add support for annotated python dicts as avro map type #433

Conversation

mariajgrimaldi commented Dec 12, 2024 • edited Loading

Description

Supporting information

Testing instructions

Deadline

Other information

Checklists

openedx-webhooks commented Dec 12, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ian2012 left a comment

Choose a reason for hiding this comment

felipemontoya left a comment

Choose a reason for hiding this comment

mariajgrimaldi commented Dec 20, 2024

bmtcril left a comment

Choose a reason for hiding this comment

mariajgrimaldi commented Dec 26, 2024

timmc-edx commented Dec 26, 2024

robrap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robrap Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariajgrimaldi Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariajgrimaldi Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

robrap left a comment

Choose a reason for hiding this comment

bmtcril left a comment

Choose a reason for hiding this comment

mariajgrimaldi commented Dec 12, 2024 •

edited

Loading

openedx-webhooks commented Dec 12, 2024 •

edited

Loading

robrap Jan 8, 2025 •

edited

Loading

mariajgrimaldi Jan 13, 2025 •

edited

Loading

mariajgrimaldi Jan 10, 2025 •

edited

Loading