Fixes #1232 reorganize document filter document modifier dir - #1472
Conversation
Greptile SummaryThis PR successfully reorganizes the directory structure for Key Changes
Issues Addressed from Previous ReviewAll previously identified issues have been resolved:
The refactoring maintains backward compatibility through comprehensive updates across the codebase while achieving the primary goal of making core filter/modifier imports lightweight. Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User imports filters] --> B{Import Type?}
B -->|Base Classes| C[filters.__init__]
B -->|Heuristic| D[filters.heuristic]
B -->|Heavy Deps| E[Opt-in Subpackages]
C -->|Lightweight| C1[DocumentFilter<br/>Filter<br/>Score<br/>ScoreFilter]
D -->|Lightweight| D1[String Filters]
D -->|Specialized| D2[heuristic.code]
D -->|Specialized| D3[heuristic.repetition]
E -->|Lazy Load| E1[filters.token<br/>TokenCountFilter]
E -->|Lazy Load| E2[filters.histogram<br/>HistogramFilter]
E -->|Lazy Load| E3[filters.fasttext<br/>FastText*Filter]
E1 -.->|__getattr__| F1[Heavy: transformers<br/>huggingface_hub]
E2 -.->|__getattr__| F2[Heavy: requests<br/>platformdirs]
E3 -.->|__getattr__| F3[Heavy: fasttext]
style C1 fill:#90EE90
style D1 fill:#90EE90
style F1 fill:#FFB6C6
style F2 fill:#FFB6C6
style F3 fill:#FFB6C6
style E1 fill:#87CEEB
style E2 fill:#87CEEB
style E3 fill:#87CEEB
Last reviewed commit: f6aeeb6 |
Additional Comments (2)
This script imports |
Additional Comments (1)
Also appears in: |
sarahyurick
left a comment
There was a problem hiding this comment.
Tysm @KunalSachdev2005 ! I left a couple suggestions, let me know what you think.
5a7e776 to
a5887c7
Compare
Additional Comments (1)
In |
|
/ok to test 2a9a601 |
|
Hi @sarahyurick, I see that two checks are failing:
|
Hi @KunalSachdev2005 sorry for the confusion about 1. Thanks. For 2, you don't have to worry about it. The Codecov check can be flaky. I can still merge it even if it continues to fail. |
…date secrets baseline (NVIDIA-NeMo#1232) Signed-off-by: Kunal Sachdev <kunalmgsachdev@gmail.com>
c002192 to
5f925f9
Compare
Additional Comments (1)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
|
Hi @sarahyurick, I've fixed the secrets detector issue - just to be safe, I replaced the |
|
/ok to test f6aeeb6 |
|
@sarahyurick The Secrets detector test is passing now! However, I see that a few other tests are failing. Looking at the logs:
Please let me know if you'd like me to do anything else at this time, but from the logs the errors seem infra-related rather than caused by my refactor/changes. |
No worries, it looks like it is just GitHub CI weirdness. I am rerunning them now. Nothing needed from your side. Thank you for the contribution @KunalSachdev2005 ! |
Description
Fixes: #1232
Importing
DocumentFiltereagerly imports many filters, some of which have heavy dependencies -TokenCountFilter(transformers; HuggingFace deps),HistogramFilter(downloads; cache), andFastTextLangId/FastTextQualityFilter.We want to make the
DocumentFilterimport lightweight. Users must opt in for heavier filters explicitly.Additionally,
DocumentFilterand specific filters are defined undernemo_curator/stages/text/filterswhileFilter/Score/ScoreFilterare undernemo_curator/stages/text/modules.DocumentModifierand other modifiers are undernemo_curator/stages/text/modifierswhileModifyis undernemo_curator/stages/text/modules.This is confusing from a user perspective.
New Directory structures in this PR:
Usage
Filters
Modifiers
Checklist
tests/stages/common/,tests/stages/text/modules/, andtests/pipelines/. All of these pass :)