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

implement LLM-based recommenders #941

Merged
merged 6 commits into from
Feb 14, 2025
Merged

implement LLM-based recommenders #941

merged 6 commits into from
Feb 14, 2025

Conversation

zhenghaoz
Copy link
Collaborator

No description provided.

Copy link

@Ap3lsin4k Ap3lsin4k left a comment

Choose a reason for hiding this comment

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

As this is refactoring, I verified that no logic was altered.

It seems like a simple Inline Function. Loop was moved from the function to the caller.

At first, I was surprised that during refactoring you need to update tests. But I gues it's ok for a public function.

Also, I like this PR because it removes if staments from unit tests making them simpler to read.

BTW changing signature of a public func is a breaking change for clients of the SDK. So this PR won't be merged now, but rather with the next major version.

Test that pass on master also pass on this branch. (Pipeline succeeds, some tests fail locally for me).

logics/item_to_item.go Show resolved Hide resolved
logics/item_to_item_test.go Show resolved Hide resolved
@zhenghaoz
Copy link
Collaborator Author

As this is refactoring, I verified that no logic was altered.

It seems like a simple Inline Function. Loop was moved from the function to the caller.

At first, I was surprised that during refactoring you need to update tests. But I gues it's ok for a public function.

Also, I like this PR because it removes if staments from unit tests making them simpler to read.

BTW changing signature of a public func is a breaking change for clients of the SDK. So this PR won't be merged now, but rather with the next major version.

Test that pass on master also pass on this branch. (Pipeline succeeds, some tests fail locally for me).

Thank you for your review. This PR is still a work in progress. Anyway, thanks.

@zhenghaoz zhenghaoz changed the title Refactor item-to-item recommender implement LLM-based recommenders Feb 14, 2025
@zhenghaoz zhenghaoz merged commit b349ba0 into master Feb 14, 2025
10 checks passed
@zhenghaoz zhenghaoz deleted the gen_ai branch February 14, 2025 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants