-
Notifications
You must be signed in to change notification settings - Fork 48
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
filter on loading rather than increase loading limit #584
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #584 +/- ##
==========================================
+ Coverage 88.55% 88.59% +0.03%
==========================================
Files 87 87
Lines 7858 7876 +18
==========================================
+ Hits 6959 6978 +19
+ Misses 899 898 -1 ☔ View full report in Codecov by Sentry. |
d24c938
to
89b6316
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea. Could we do it with expression rather then lambda? What sould be with other loaders which are not HFLoader? Lastly maybe we should consider doing it all through an independent operator?
89b6316
to
4e01d1e
Compare
My understanding of the HF api, is that a function returning True or False, when fed with an insrance, is needed there. We need it to be artifact-able, so can be cataloged. Lambda seemed most apropriate. I see that currently, the load limits sneak into the loader, and become effective as it actually loads. Is it advisable to change this too? |
c803772
to
a6becb2
Compare
Signed-off-by: dafnapension <dafnashein@yahoo.com>
…overage Signed-off-by: dafnapension <dafnashein@yahoo.com>
a6becb2
to
3818172
Compare
per @elronbandel comment in recent PR:
Perhaps employing the HuggingFace filter upfront can ease load on the system, better than increase loading_limit only to allow the following pre-processing step that filters out.