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

Fully merge the Watson and Embeddings classification features #709

Merged
merged 22 commits into from
Feb 19, 2024

Conversation

dkotter
Copy link
Collaborator

@dkotter dkotter commented Feb 9, 2024

Description of the Change

Historically (though I use that term loosely since the Embeddings functionality hasn't been around that long) we've had two classification Features:

  1. Analyze your content and suggest new terms using the Watson API
  2. Analyze your content and existing terms, suggesting which of the existing terms are the best match using the OpenAI Embeddings API

Coming out of the work in #611, these two separate features were merged down into a single Feature, with Watson and Embeddings being two different providers for that Feature.

But we left most of the functionality in each individual Provider class instead of bringing that into the Feature class. These features are very similar but have diverged a bit over time so there wasn't an easy, clean merge that could be done.

The ultimate goal was to merge those two together and bring parity between the two and that is what this PR does. All shared functionality has been moved from the individual Provider classes into the Classification Feature class. Any functionality that was missing between the two (like the modal popup we added to Watson but never to Embeddings) has been taken care of. In theory, the Classification Feature should now function basically the same no matter the Provider that is selected.

Closes #671
Closes #699
Closes #714

How to test the Change

I would suggest fully testing the Classification Feature with both Providers. This could involve the following:

  • Ensure classifying content works in the Classic Editor, both with the checkbox checked and clicking the classify button
  • Ensure classifying content works in the Block Editor, both with the checkbox checked and clicking the classify button
  • Ensure classifying an individual item works from the post list screen by clicking the inline link
  • Ensure classifying one or more items works from the post list screen by using the bulk actions dropdown

Changelog Entry

Added - Ability to preview which terms will be added before they get added when using the Embeddings provider.
Changed - Ensure the Classification functionality works the same for all Providers.
Fixed - Fix fatal error if both the Classification Feature and Moderation Features are active.

Credits

Props @dkotter

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@dkotter dkotter added this to the 3.0.0 milestone Feb 9, 2024
@dkotter dkotter self-assigned this Feb 9, 2024
@dkotter dkotter requested review from jeffpaul and a team as code owners February 9, 2024 16:19
@dkotter dkotter linked an issue Feb 9, 2024 that may be closed by this pull request
1 task
@github-actions github-actions bot added the needs:code-review This requires code review. label Feb 9, 2024
@dkotter dkotter requested review from iamdharmesh and removed request for a team and jeffpaul February 13, 2024 15:37
Copy link
Member

@iamdharmesh iamdharmesh left a comment

Choose a reason for hiding this comment

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

@dkotter, Thanks a lot for merging this. The code LGTM and it tests well. However, I have added two minor suggestions; otherwise, everything looks good.

Also, I have noticed that when we use embeddings as a provider, it also suggests the 'Uncategorized' category as a matching term. I'm not sure if this happens in my setup only. Could you please check once? If it suggests 'Uncategorized,' then we might want to filter it from the suggestions list.

includes/Classifai/Admin/BulkActions.php Outdated Show resolved Hide resolved
@dkotter
Copy link
Collaborator Author

dkotter commented Feb 16, 2024

Also, I have noticed that when we use embeddings as a provider, it also suggests the 'Uncategorized' category as a matching term. I'm not sure if this happens in my setup only. Could you please check once? If it suggests 'Uncategorized,' then we might want to filter it from the suggestions list

Saw the same thing. This should now be excluded

Copy link
Member

@iamdharmesh iamdharmesh left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@dkotter I have added 2 minor commits to fix #714 and Exclude the "Post Format" taxonomy from the supported taxonomies list. This is ready to merge now. Thank you.

@dkotter dkotter merged commit 4d02df0 into develop Feb 19, 2024
13 checks passed
@dkotter dkotter deleted the feature/671 branch February 19, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment