Bundles xapi-db-load into a command-line tool with setuptools #1

pomegranited · 2023-01-25T10:41:51Z

...and handles a breaking change with query settings from clickhouse-connect==0.5.0.

Related discussions

Part of implementing Spike: Integrate Superset with Clickhouse storing xAPI events openedx-aspects#30

Testing instructions

E.g inside a docker container with access to clickhouse_service from https://github.com/bmtcril/tutor-contrib-clickhouse :

Checkout this branch, and change into the repo directory.
In a new python virtualenv, run pip install . to install it and the latest dependencies.
Run xapi-db-load --backend clickhouse... to ensure that the script still runs as expected.

Author Notes & Concerns

@bmtcril Can you tag this as 0.1 after merging? Then I can reference it in my tutor init scripts, and we don't have to worry about api changes on the script.

pomegranited · 2023-01-25T10:43:01Z

@bmtcril Are you ok with these changes? I had to make them in order to use this code to (optionally) loading demo xAPI data with my OARS tutor plugin.

pomegranited · 2023-01-25T11:22:20Z

@bmtcril and out of interest, I assume that the xAPI schemas used here more closely match Ralph's not the event-routing-backends?

I will make my Superset permissions filter flexible so it will suit any (flat) xAPI course field imported into Clickhouse, but was just wondering for my own notes.

bmtcril · 2023-01-25T16:31:36Z

I'll have to dig through and see what I've got pending. I think this is fine, I just want to make sure any big refactors I've done have already landed otherwise it might be easier to land them first then rebase.

The events generated here are straight from the event-routing-backends tests right now: https://github.com/openedx/event-routing-backends/tree/master/event_routing_backends/processors/xapi/tests/fixtures/expected

There are definitely some things that can be improved, especially w/r/t ordering of events. Right now I think it enrolls learners in courses before firing other events for them, but everything else happens in a random order and will make for some extremely confusing visualizations.

bmtcril · 2023-01-25T17:47:13Z

Sadly I did have a large branch running for the Ralph tests. If you want to take a spin through it here, I can merge it and then fight through the rebase on this PR to get it in: #2

pomegranited · 2023-01-25T22:43:43Z

@bmtcril I'm more than happy to rebase -- my changes are simple, they only look big. Please let me know when you've merged your major work, and I'll reapply my changes.

bmtcril · 2023-01-26T14:05:31Z

Ok, I've merged my big mess and will start treating this as a real shared repo now. Thanks!

pomegranited · 2023-01-31T00:16:01Z

xapi_db_load/backends/clickhouse_lake.py

@@ -20,9 +20,10 @@ def __init__(self, db_host="localhost", db_port=18123, db_username="default",
            password=db_password,
            port=self.port,
            database=self.database,
-            date_time_input_format="best_effort",  # Allows RFC dates
-            old_parts_lifetime=10,  # Seconds, reduces disk usage


@bmtcril I get this error when I include 'old_parts_lifetime': 10 in the client settings:

clickhouse_connect.driver.exceptions.ProgrammingError: Setting old_parts_lifetime is unknown or readonly

So I've added it to the MergeTable CREATE statement below instead. From what you've learned about Clickhouse, is that ok?

That should be fine here. I wonder if there's a version mismatch in one of our installs, though. If we're going to be using this elsewhere we should probably do the work to make it an officially supported repo. I'll ask around about what people think about that.

@bmtcril Or maybe we need to pin the version of clickhouse-connect? I've got this, what are you running?

clickhouse-connect==0.5.3

I've got 0.3.6 so am on the other side of this breaking change: https://github.com/ClickHouse/clickhouse-connect/blob/main/CHANGELOG.md#050-2023-01-14

I think pinning it to >0.5 and <0.6 is probably the best since they seem to do breaking changes on minor versions.

pomegranited · 2023-01-31T00:53:16Z

Thanks for merging your changes @bmtcril ! I've rebased and re-tested, and fixed some things (see individual commits).
So this PR and openedx-unsupported/tutor-contrib-clickhouse#1 are ready for your review :)

I'm working on a 3rd PR against https://github.com/open-craft/tutor-contrib-superset with scripts to initialize the clickhouse data source, and add a chart + dashboard for https://github.com/openedx/data-wg/issues/26.

README.rst

bmtcril · 2023-01-31T13:59:18Z

xapi_db_load/backends/clickhouse_lake.py

-            -- video_id String NULL,
-            -- nav_starting_point String NULL,
-            -- nav_ending_point String NULL,
+            course_id String NOT NULL,


These changes are all fine, this is mostly a note for myself to come back and change the rest of this create statement to the one that was performing better in the Ralph tests:

CREATE TABLE IF NOT EXISTS {self.event_table_name} ( event_id UUID NOT NULL, verb_id String NOT NULL, actor_id UUID NOT NULL, org String NOT NULL, course_id String NOT NULL, emission_time timestamp NOT NULL, event JSON NOT NULL ) ENGINE MergeTree ORDER BY (org, course_id, verb_id, actor_id, emission_time, event_id) PRIMARY KEY (org, course_id, verb_id, actor_id, emission_time, event_id)

Ah cool -- I've left the extra fields alone (since the data loading requires them), but c106f35 adds those additional fields to ORDER BY and PRIMARY KEY.

Ok, thanks. I'll normalize those between the clickhouse and Ralph backends in a different PR. That table structure is going to all need to be updated after some discussions I just had with FUN, but I haven't decided on exactly how yet.

bmtcril

This is great, I left some comments but I think the only real change needed is to the help string.

bmtcril · 2023-02-01T19:40:57Z

Just like the other one, @pomegranited this repository has been moved into the openedx GitHub org. Can you confirm here that you're good with contributing this PR to openedx under your CLA?

pomegranited · 2023-02-02T00:13:41Z

@bmtcril I've addressed your suggestions, please have a look?

Thanks for moving this repo into the openedx fold :)
I'm happy to contribute this PR to openedx under my CLA. Do I need to also request merge access to it as part of my CC role, and get this and the other OARS repos added to the "analytics" group?

bmtcril

I'm happy to approve this as-is and resolve the outstanding comments in a follow-on PR so as to unblock openedx-unsupported/tutor-contrib-clickhouse#1

README.rst

bmtcril · 2023-02-02T14:25:22Z

xapi_db_load/backends/clickhouse_lake.py

@@ -20,9 +20,10 @@ def __init__(self, db_host="localhost", db_port=18123, db_username="default",
            password=db_password,
            port=self.port,
            database=self.database,
-            date_time_input_format="best_effort",  # Allows RFC dates
-            old_parts_lifetime=10,  # Seconds, reduces disk usage


I've got 0.3.6 so am on the other side of this breaking change: https://github.com/ClickHouse/clickhouse-connect/blob/main/CHANGELOG.md#050-2023-01-14

I think pinning it to >0.5 and <0.6 is probably the best since they seem to do breaking changes on minor versions.

bmtcril · 2023-02-02T14:27:21Z

xapi_db_load/backends/clickhouse_lake.py

-            -- video_id String NULL,
-            -- nav_starting_point String NULL,
-            -- nav_ending_point String NULL,
+            course_id String NOT NULL,


Ok, thanks. I'll normalize those between the clickhouse and Ralph backends in a different PR. That table structure is going to all need to be updated after some discussions I just had with FUN, but I haven't decided on exactly how yet.

bmtcril · 2023-02-02T14:45:50Z

@pomegranited I added you as a grandfathered-in "writer" to this repo and tutor-contrib-clickhouse since you were already working on them. I'm happy to sponsor you to the analytics CC group, which would likely make all the management easier. These repos are all in there as well, if you're ok with it I'll make a post about it in the forums!

openedx-webhooks · 2023-02-02T20:13:47Z

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.

pomegranited · 2023-02-06T05:37:46Z

@bmtcril

I added you as a grandfathered-in "writer" to this repo and tutor-contrib-clickhouse since you were already working on them.

Thank you :) I'll merge this, and get it tagged.

I'm happy to sponsor you to the analytics CC group, which would likely make all the management easier. These repos are all in there as well, if you're ok with it I'll make a post about it in the forums!

Thank you, yes please! I'm already a core contributor to the legacy analytics repos, but this was granted before we had these CC repo groups.

Modularizes the source code so that it can be installed and run as a command line script, and adds missing dependencies.

Clickhouse client no longer accepts arbitrary settings with each request. cf https://github.com/ClickHouse/clickhouse-connect/blob/main/CHANGELOG.md#warning----breaking-change----removing-get_client-arbitrary-keyword-arguments * Retains the date_time_input_format as a general setting for all calls * Moves old_parts_lifetime to the MergeTree table creation because errors are thrown if it's used by the client on non-MergeTree tables: clickhouse_connect.driver.exceptions.ProgrammingError: Setting old_parts_lifetime is unknown or readonly * Ensures events table gets created with all the expected fields * Tweaks the order_by and primary key fields for performance

pomegranited · 2023-02-06T05:50:24Z

@bmtcril Ach, sorry.. I squashed commits and git wants a new approving review. (Tried reverting back to the previously approved commit, and it didn't buy it.) I'll remember for next time and just squash on merge!

openedx-webhooks · 2023-02-06T13:57:48Z

@pomegranited 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

pomegranited mentioned this pull request Jan 31, 2023

[feat] Adds option to load demo xAPI data openedx-unsupported/tutor-contrib-clickhouse#1

Merged

pomegranited commented Jan 31, 2023

View reviewed changes

bmtcril reviewed Jan 31, 2023

View reviewed changes

README.rst Show resolved Hide resolved

bmtcril reviewed Jan 31, 2023

View reviewed changes

bmtcril previously approved these changes Feb 2, 2023

View reviewed changes

bmtcril changed the title ~~Bundles xapi-db-load into a command-line tool with setuptools~~ Bundle xapi-db-load into a command-line tool with setuptools Feb 2, 2023

bmtcril changed the title ~~Bundle xapi-db-load into a command-line tool with setuptools~~ Bundles xapi-db-load into a command-line tool with setuptools Feb 2, 2023

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Feb 2, 2023

pomegranited added 2 commits February 6, 2023 16:16

[feat] bundles this script with setuptools

5d34343

Modularizes the source code so that it can be installed and run as a command line script, and adds missing dependencies.

pomegranited dismissed bmtcril’s stale review via d3784be February 6, 2023 05:47

bmtcril approved these changes Feb 6, 2023

View reviewed changes

bmtcril merged commit dbc1468 into openedx:main Feb 6, 2023

bmtcril mentioned this pull request Feb 6, 2023

Updates to get the repo up to spec, fix up docs, small bug fix #4

Merged

pomegranited deleted the jill/oars-tweaks branch February 7, 2023 00:06

pomegranited mentioned this pull request Feb 7, 2023

feat: adds clickhouse-connect openedx-unsupported/tutor-contrib-superset#2

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bundles xapi-db-load into a command-line tool with setuptools #1

Bundles xapi-db-load into a command-line tool with setuptools #1

pomegranited commented Jan 25, 2023 •

edited

pomegranited commented Jan 25, 2023

pomegranited commented Jan 25, 2023 •

edited

bmtcril commented Jan 25, 2023

bmtcril commented Jan 25, 2023

pomegranited commented Jan 25, 2023

bmtcril commented Jan 26, 2023

pomegranited Jan 31, 2023

bmtcril Jan 31, 2023

pomegranited Feb 1, 2023

bmtcril Feb 2, 2023

pomegranited commented Jan 31, 2023

bmtcril Jan 31, 2023

pomegranited Feb 2, 2023

bmtcril Feb 2, 2023

bmtcril left a comment

bmtcril commented Feb 1, 2023

pomegranited commented Feb 2, 2023

bmtcril left a comment

bmtcril Feb 2, 2023

bmtcril Feb 2, 2023

bmtcril commented Feb 2, 2023

openedx-webhooks commented Feb 2, 2023

pomegranited commented Feb 6, 2023

pomegranited commented Feb 6, 2023

openedx-webhooks commented Feb 6, 2023

Bundles xapi-db-load into a command-line tool with setuptools #1

Bundles xapi-db-load into a command-line tool with setuptools #1

Conversation

pomegranited commented Jan 25, 2023 • edited

pomegranited commented Jan 25, 2023

pomegranited commented Jan 25, 2023 • edited

bmtcril commented Jan 25, 2023

bmtcril commented Jan 25, 2023

pomegranited commented Jan 25, 2023

bmtcril commented Jan 26, 2023

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

pomegranited commented Jan 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmtcril left a comment

Choose a reason for hiding this comment

bmtcril commented Feb 1, 2023

pomegranited commented Feb 2, 2023

bmtcril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmtcril commented Feb 2, 2023

openedx-webhooks commented Feb 2, 2023

pomegranited commented Feb 6, 2023

pomegranited commented Feb 6, 2023

openedx-webhooks commented Feb 6, 2023

pomegranited commented Jan 25, 2023 •

edited

pomegranited commented Jan 25, 2023 •

edited