Conversation
…ement counting logic with unit tests
|
Thanks for the pull request, @tbain! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
openedx-webhooks
added
the
open-source-contribution
jesperhodge
left a comment
jesperhodge
left a comment
There was a problem hiding this comment.
There seem to be changes missing. For example, src/taxonomy/data/api.ts.
Could you
- review this PR and make sure that all necessary changes are in this branch? Compare to the open Unicon PR.
- review discussions in the Unicon PR and either resolve them or copy them here to be addressed here.
- fix any pipeline errors
?
|
Since we're no longer using recursive SQL for this, is it possible to update the PR description for accuracy? |
|
…bain/253_add_tags_count_rebased # Conflicts: # src/openedx_tagging/models/base.py # tests/openedx_tagging/test_api.py
There was a problem hiding this comment.
Pull request overview
Adds rolled-up, de-duplicated tag usage counts (including ancestor rollups) to the tag listing query so the Taxonomies UI can display accurate “Usage Count” values per tag.
Changes:
- Replaced the prior per-tag direct usage counting subquery with a dynamic, depth-aware subquery that rolls counts up to ancestors with per-object de-duplication.
- Updated existing API/model tests to reflect rolled-up counts and added a broader set of usage-count test cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
src/openedx_tagging/models/base.py |
Centralizes and updates include_counts behavior by annotating tag querysets with rolled-up, de-duplicated usage_count via a subquery. |
tests/openedx_tagging/test_models.py |
Updates expected usage counts and adds multiple new test scenarios validating ancestor rollup and sibling de-duplication. |
tests/openedx_tagging/test_api.py |
Updates autocomplete/search test expectations to reflect rolled-up usage counts returned by the API when include_counts=True. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Feel free to ping me for review here once the AC are clarified and the comments from Copilot etc are addressed. |
@bradenmacdonald I think this is ready for re-review, I resolved all the Copilot issues and added the improvement you suggested for finding the depth via a query rather than depending on the constant |
…t & filter query to current taxonomy
|
When I test this using |
|
I'm not opposed to this PR as is, but a 10x slowdown isn't great, and I suspect it may be worse if there are more ObjectTags in use (I don't have that many in my test environment). In order to improve performance, I have two suggestions:
Thoughts? |
I think I like option 2 better as well because I think it's clearer that it will help with the performance and likely take less implementation time. However, unfortunately, I think even if option 2 is only about a 2ish day effort to implement, the number of fast follows we've been promising are starting to stack up, so we're getting a bit more concerned about our timeline. I'd like to add a new Github Issue for our "Nice to Haves" to address the performance concerns here and proceed with merging as is, if possible @bradenmacdonald ? Some other related thoughts I have from a big picture use case perspective: We don't anticipate taxonomies much larger than the Lightcast sample. However, I do anticipate that for folks who create new course runs every term and have very short terms, the number of ObjectTag associations could get pretty large. I think that this is where it could potentially be valuable to add a filter to the tag count to only fetch ObjectTags where the Object corresponds to a course that is currently running or will be running in the future. I think the rest of the big picture for this is that for the folks who create new course runs every term and have very short terms, the usage count is probably meaningless for them and not very helpful anyways "Was it used 250 times or 275 times? How do I know how much usage I should be expecting compared to what I'm seeing?" I think there could also be an option to just hide the usage count column altogether if we detect that their usages are so high that this info is irrelevant for the instance. Or to hide the usage count for people who primarily use course runs each term instead of continuously running courses. |
|
We're trying to stabilize the APIs for Verawood, so if we think we're ultimately going to end up with a second API endpoint for getting the counts, then I'd prefer to split that off separately now, even if we just use the existing implementation exactly as it is in this PR. In that case, it would definitely take less than 2 days, because you don't have to change much (although you could simplify it if you get time). You can also mark that "counts" endpoint as unstable so we can freely evolve it in Willow while keeping the "get tags" endpoint stable.
That all makes a lot of sense, but will require a lot of discussion, because the current API is not aware of "courses" as a concept at all, and I'm a bit reluctant to make the tagging API aware of those things - right now tagging is a very low-level feature that other things build on. If you're even considering functionality like that, then I think it's another reason to move the tag usage counts to a separate endpoint, where it can support more elaborate options/filtering. @ormsbee Can I get your thoughts? |
|
@bradenmacdonald if I understand correctly:
Did I get that correctly? So can we consider this PR unblocked in this case? |
…bain/253_add_tags_count_rebased # Conflicts: # tests/openedx_tagging/test_api.py
|
@bradenmacdonald @tbain here is the new issue. I have not worked out an accurate title or description for it, so you can just edit the issue however you see fit. |
This implements openedx/modular-learning#253 , the task to add tag usage counts to the tags table under the taxonomies table. The corresponding backend part is openedx/openedx-core#506, which updates the count aggregations to ensure the correct count numbers are sent to the frontend. This frontend PR does not depend on the backend part.
What I meant was that we should change the PR to provide the desired tag count data via a separate endpoint. But using more or less the exact same code as you have now if you don't want to refactor it. So instead of But I guess that's going to require some major changes on the frontend side to combine those pieces of information, so maybe that's not going to work with your timeline. |
|
I guess before we consider merging this as is, I'd like to know if the slowness mostly scales with taxonomy size or object tag count or both? If the slowness is only a factor on large taxonomies and it's just ~1s, I think that's OK for now. But if it's slow as the # of object tags increases or it's O(n_tags * n_object_tags) or anything like that, then it'll seem fine now and slow to a crawl in prod once people start using thousands of these things and re-running tagged courses. |
I agree with this. If it's ~1s for an outlier taxonomy owing to the number of tags, it's acceptable for now, and we can figure out how to optimize later. If the time scales with the number of things tagged, this will rapidly become unusable.
I'd be cautious about assuming people don't care. I've been told that there's sometimes grant money riding on proving how much things get used. In any case, we'd definitely need product folks to weigh in on it. |
|
FWIW Claude analyzed the query and says it could be slow. I have not had time to validate this analysis, so take with a grain of salt. The generated SQL (at depth=3)SELECT ...,
COALESCE(
(SELECT COUNT(DISTINCT U0."object_id") AS "total_usage"
FROM "oel_tagging_objecttag" U0
INNER JOIN "oel_tagging_tag" U2 ON (U0."tag_id" = U2."id")
LEFT OUTER JOIN "oel_tagging_tag" U3 ON (U2."parent_id" = U3."id")
LEFT OUTER JOIN "oel_tagging_tag" U4 ON (U3."parent_id" = U4."id")
WHERE U0."taxonomy_id" = 1
AND (U0."tag_id" = outer."id"
OR U2."parent_id" = outer."id"
OR U3."parent_id" = outer."id"
OR U4."parent_id" = outer."id")
), 0) AS "usage_count"
FROM "oel_tagging_tag"
WHERE "oel_tagging_tag"."taxonomy_id" = 1The scaling problem: it will get meaningfully slowerThe old query filtered by The new query is a correlated subquery that, for each tag in the result set, does this:
Cost per tag: O(all_ObjectTags_in_taxonomy × D) So the total work is roughly T × O × D where:
It scales linearly with ObjectTag count, but since it's inside a correlated subquery that runs per-tag, the multiplier is the number of tags displayed. This will be painfully slow once a popular taxonomy gets applied to thousands of courses/modules/sections. Why the OR kills performanceThe condition |
|
Okay. So it sounds like the most straightforward thing is to do the up-front query for counts and stitch together the hierarchy counts in Python as @bradenmacdonald outlined in:
Does that sound right to everyone? |
|
That sounds good to me, and has the advantage of requiring no further changes to the frontend PR. |
|
@ormsbee @bradenmacdonald the only question I have is related to memory usage. |
|
@bradenmacdonald @ormsbee just to make sure we have considered all alternatives: AI is suggesting Recursive CTEs as the optimal solution. However, that requires MySQL >= 8. Do we need to support older MySQL versions? I haven't been able to evaluate the AI response in-depth so it may be incorrect AI suggestion:
N = Number of Tags in your main queryset |
…nstead of via expensive db query
|
Ultimately, via a conversation/clarification over Slack (dated 2026-04-02), we decided to address the performance concerns via in-memory python based code processing rather than trying to rely on django joins and sub-queries, or a recursive SQL/CTE implementation. Since we were seeing such an egregious performance hit, the implementation leans towards minimizing performance issues and bottlenecks where possible, potentially at the slight cost of straight-forwardness of what exactly the code is doing (e.g. performance wise it was very expensive to implement the 'annotation' of the |
|
Did some local testing with the large Lightcast taxonomy that Braden posted earlier; applied some tags from that taxonomy to an existing course on my local, and then watched the timings for the Various load times with Lightcast Taxonomy: However, I'm not quite sure this is the best representation of the times to reproduce the same circumstances as Braden saw above with the 10x increase in call time, since I don't have the same tags applied the same way to the same depth, the same course, etc. Also I have a brand new computer that is very fast, which is kind of throwing this off as well. I only have a handful of tags applied; if I could either get some direction from Braden on how he had applied his tickets, or have Braden perform a quick check with his same setup, that would be great. |
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
Thanks! My performance concern is addressed now. I just caught a few more things but hopefully they're relatively straightforward to address.
| When using get_filtered_tags() with both a search_term and | ||
| include_counts=True, the usage_count returned should still | ||
| reflect the true count for each matching tag, not be affected | ||
| by the search filter. | ||
| """ | ||
| api.tag_object("obj:1", self.taxonomy, [self.eubacteria.value]) | ||
| api.tag_object("obj:2", self.taxonomy, [self.archaebacteria.value]) | ||
| result = pretty_format_tags( | ||
| self.taxonomy.get_filtered_tags(search_term="bacteria", include_counts=True) | ||
| ) | ||
| assert result == [ | ||
| "Bacteria (None) (used: 2, children: 2)", | ||
| " Archaebacteria (Bacteria) (used: 1, children: 0)", | ||
| " Eubacteria (Bacteria) (used: 1, children: 0)", |
There was a problem hiding this comment.
This test doesn't seem to be testing what it says. It says "the usage_count returned should still reflect the true count for each matching tag, not be affected by the search filter." But all of the results are matching the search filter ("bacteria"). I think what you need to do is use a search filter that matches only the parent tags and excludes the children, but show that the children's count is still included in the parents, even though the children are not part of the current filtered result set.
| Tagging an object with a depth-3 tag (Chordata) should roll up | ||
| to grandparent (Animalia) and great-grandparent (Eukaryota), | ||
| verifying the full 3-level lineage query in add_counts_query. |
There was a problem hiding this comment.
The description of this test is not accurate and references a function that was refactored/renamed.
| Tagging an object with a depth-3 tag (Chordata) should roll up | |
| to grandparent (Animalia) and great-grandparent (Eukaryota), | |
| verifying the full 3-level lineage query in add_counts_query. | |
| Tagging an object with a depth-3 tag (Chordata) as well as one | |
| of that tag's parents (Animalia) should roll up as only a single | |
| tag usage at the grandparent level, because the parent tag | |
| (Animalia) is implied by the child tag (Chordata) anyways. |
| When listing children of a tag (depth=1, parent_tag_value=...), the | ||
| usage_count of each child should only reflect the objects tagged with | ||
| that child or any of its descendants. | ||
| """ | ||
| api.tag_object("obj:1", self.taxonomy, [self.mammalia.value]) # grandchild of Animalia via Chordata | ||
| api.tag_object("obj:2", self.taxonomy, [self.chordata.value]) # direct child of Animalia | ||
| result = pretty_format_tags( | ||
| self.taxonomy.get_filtered_tags(depth=1, parent_tag_value="Animalia", include_counts=True) | ||
| ) | ||
| assert result == [ | ||
| " Arthropoda (Animalia) (used: 0, children: 0)", | ||
| " Chordata (Animalia) (used: 2, children: 1)", | ||
| " Cnidaria (Animalia) (used: 0, children: 0)", | ||
| " Ctenophora (Animalia) (used: 0, children: 0)", | ||
| " Gastrotrich (Animalia) (used: 0, children: 0)", | ||
| " Placozoa (Animalia) (used: 0, children: 0)", | ||
| " Porifera (Animalia) (used: 0, children: 0)", | ||
| ] |
There was a problem hiding this comment.
the
usage_countof each child should only reflect the objects tagged with that child or any of its descendants.
You're not really testing that we only reflect child/descendant tags if you only create child/descendant tags. I suggest that you also apply some tags using the parent (Animalia) as well as some entirely unrelated tags, and make sure neither is affecting the count. Or remove the word "only" from the description.
I think it would also help to call out # Tag two different objects: at the start to better distinguish this text from the following test.
| tag_lineage_dict = dict(self.tag_set.all().filter(taxonomy_id=self.id).values_list("value", "lineage")) | ||
| object_tags = self.objecttag_set.all().filter(taxonomy_id=self.id).values_list("_value", "object_id") | ||
| tag_counts: Counter[str] = Counter() | ||
| object_tag_lineage_seen: defaultdict[str, set] = defaultdict(set) | ||
|
|
||
| for tag_value, object_id in object_tags: | ||
| # split the lineages to get a dict of {tag.value: [lineages]} | ||
| lineage_tags = (t for t in tag_lineage_dict.get(tag_value, "").split('\t') if t) |
There was a problem hiding this comment.
First, self.tag_set.all().filter(taxonomy_id=self.id) is the same as self.tag_set, so you could simplify each of these. But secondly, you don't need to pre-load all the lineage values for the whole taxonomy as a separate query. Just grab the lineage values for the actually used tags when you're loading the ObjectTags, and this should be way more efficient:
- tag_lineage_dict = dict(self.tag_set.all().filter(taxonomy_id=self.id).values_list("value", "lineage"))
- object_tags = self.objecttag_set.all().filter(taxonomy_id=self.id).values_list("_value", "object_id")
+ object_tags = self.objecttag_set.values_list("object_id", "tag__lineage")
tag_counts: Counter[str] = Counter()
object_tag_lineage_seen: defaultdict[str, set] = defaultdict(set)
- for tag_value, object_id in object_tags:
+ for object_id, tag_lineage in object_tags:
# split the lineages to get a dict of {tag.value: [lineages]}
- lineage_tags = (t for t in tag_lineage_dict.get(tag_value, "").split('\t') if t)
+ lineage_tags = [t for t in tag_lineage.split('\t')] if tag_lineage else []
# de-duplicate based on if the lineage is already 'seen' per object
unseen_tags = [t for t in lineage_tags if t not in object_tag_lineage_seen[object_id]]| ) | ||
| ) | ||
| qs = qs.annotate(usage_count=models.Subquery(obj_tags.values("count"))) | ||
| return self._add_counts(list(cast(list, qs))) # type: ignore[return-value] |
There was a problem hiding this comment.
Can we move this _add_counts annotation to happen at the REST API level, after the queryset has already been evaluated? Because the purpose of this API returning a queryset was so that the REST API or other code can paginate it, filter it, and do other things with the QuerySet, but you have now changed it to return a full list of TagData objects, not a QuerySet at all.
To achieve that, we may need to remove the include_counts option from this python function and just make it available as a separate annotate_counts API.
Not asking for changes to your annotation function, just considering using it in a different place to keep the API flexible. If we did keep it here, you must update the declared return type of the function, because we really can't have a function that says it returns a TagDataQuerySet but actually returns a list.
Description
This implements openedx/modular-learning#253 , the task to add tag usage counts to the tags table under the taxonomies table. The frontend piece is where the results of this aggregation work is displayed is part of a separate pr to openedx/frontend-app-authoring. This change adds a subquery annotation onto the django query for retrieving tags. The original implementation of the counts for tags only counted raw usage of each tag. This feature/PR aggregatea sum of any tag and child tag usage with sibling de-duplication for the same usage (e.g. when two sibling nodes are used against the same course, module, etc. we still only need to count that as '1' for any parent/grandparent nodes) as specified in the AC for the issue above, so it was replaced with this more complicated bit of logic that sums across tag usage based on various courses, sections, modules, and libraries that might use a tag.
The count logic is done in-memory, since we saw noticeable performance issues with trying to stay in the QuerySet/Django paradigm for calculating the counts. This makes the code a little less straightforward, since we break it out into a somewhat odd in-memory python application of the logic, but it still works as intended and resolves as many performance pain points as possible while still adhering to the counting requirements that end up necessitating such code.
AI Usage Disclosure: Claude was used via intelliJ IDE integration was used through the authoring process to work through complicated logic, and also simplify it/make it more pythonic/alleviate performance concerns.
Supporting information
Github issue with AC: openedx/modular-learning#253
Testing instructions
Refer to the AC in the Github Issue. Steps to verify this is implemented and working via UX (Note, depends on the frontend part of this ticket):
Other information
Include anything else that will help reviewers and consumers understand the change.