Skip to content

Jinja - support rejectattr #988

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

Merged
merged 3 commits into from
Oct 30, 2024
Merged

Jinja - support rejectattr #988

merged 3 commits into from
Oct 30, 2024

Conversation

giladgd
Copy link
Contributor

@giladgd giladgd commented Oct 27, 2024

I've added support for rejectattr in Jinja since I encountered some GGUF models that include a chat template that uses it, like this one for example.

@giladgd giladgd requested a review from xenova as a code owner October 27, 2024 02:06
Copy link
Contributor

@xenova xenova left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! 🎉

Since rejectattr is very similar to selectattr, maybe we can remove code redundancy by reusing the logic?

Something like:

case "selectattr": 
case "rejectattr": 
{
const select = filterName === "selectattr";

// ... same logic

// Filter the array using the test function
const filtered = (operand.value as ObjectValue[]).filter((item) => {
    const a = item.value.get(attr.value);
    const result = a ? testFunction(a, value) : false;
    return select ? result : !result;
});

}

the following should do the same (less readable though)

const filtered = (operand.value as ObjectValue[]).filter(item => {
    const a = item.value.get(attr.value);
    return select === !!(a && testFunction(a, value));
});

@giladgd giladgd requested a review from xenova October 27, 2024 20:19
@giladgd
Copy link
Contributor Author

giladgd commented Oct 29, 2024

I've made the changes, and the tests seem to pass 👍🏻

Copy link
Contributor

@xenova xenova left a comment

Choose a reason for hiding this comment

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

A perfect PR! Thanks a ton! ✅

Will put out a new release shortly too (just needing to finish one more thing)

@xenova xenova merged commit cf67172 into huggingface:main Oct 30, 2024
4 checks passed
@giladgd giladgd deleted the jinjaAttr branch October 30, 2024 17:12
@julien-c
Copy link
Member

julien-c commented Nov 1, 2024

thanks for the contrib, @giladgd!

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.

3 participants