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

Annotate channels with some ordered metadata #12944

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

jredrejo
Copy link
Member

@jredrejo jredrejo commented Dec 13, 2024

Summary

  • In ChannelMetadata, it creates two new fields:
    • included_categories
    • included_grade_levels
    • and changes type of included_languages from ManyToManyField to a special type (from a new added library dependency) called SortedManyToManyField
  • Creates migrations for these fields
  • Creates and/or modify ChannelMetadata annotations
    • calculate_included_languages
    • calculate_ordered_categories
    • calculate_ordered_grade_levels
  • Adds tests for these changes
  • Adds an upgrade method, ordered_metadata_in_channels, for previously annotated channels (Important note: this has to be skipped in KDP migrations)

References

Fixes: #12880

Reviewer guidance

  • do tests pass
  • try to import channels, totally or partially and check their included_categories, included_grade_levels and included_languages are updated correctly

@jredrejo jredrejo requested a review from rtibbles December 13, 2024 18:37
@github-actions github-actions bot added the DEV: backend Python, databases, networking, filesystem... label Dec 13, 2024
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

If you can refer to the big comment at the top of models.py - we want to run this command:

kolibri manage generate_schema current

and commit the result as part of this PR too.


value_counts = {}
for value in all_values:
value_counts[value] = value_counts.get(value, 0) + 1
Copy link
Member

Choose a reason for hiding this comment

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

This is a beautifully concise way to default the value to 0.

@rtibbles
Copy link
Member

I thought I had fixed this last remaining flaky content test failure on postgres, but it still happened! Have rerun it.

@jredrejo jredrejo marked this pull request as ready for review December 13, 2024 21:33
@rtibbles
Copy link
Member

Looking good to me! I'll give it a bit more of a close review on Monday. If you can do the schema regeneration, I think this should be quickly mergeable.

…tible with python 3.12, sqlacodegen is trying to import ArgSpec from the inspect module, but this has been removed in Python 3.12
@jredrejo
Copy link
Member Author

If you can refer to the big comment at the top of models.py - we want to run this command:

kolibri manage generate_schema current

done, but had to modify a bit generate_schema due to python 3.12 compatibility issues

inspect.ArgSpec = ArgSpec
inspect.getargspec = getargspec

# SQLAlchemy 2.0 compatibility patch
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need a compatibility patch with SQLAlchemy 2? We explicitly pin to less than 2, and I've not had any issues installing SQLAlchemy 1.4.49 in Python 3.12.

@@ -45,6 +46,9 @@ class ContentLocalfile(Base):
class ContentContentnode(Base):
__tablename__ = "content_contentnode"
__table_args__ = (
CheckConstraint(
Copy link
Member

Choose a reason for hiding this comment

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

Adding the constraint checks seems to be causing the Python test errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: backend Python, databases, networking, filesystem...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Annotate ordered channel metadata during content annotation
2 participants