docs: Add HebPipe architecture and migration report#1
Conversation
- Added `doc/report.txt` containing a detailed architecture and migration report. - The report covers project overview, data/logic analysis, gaps, migration plan, and recommendations for improvement. Co-authored-by: arielnahom <16294824+arielnahom@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a detailed re-architecture and migration report for the HebPipe Hebrew NLP pipeline, covering project overview, data assets, and a modernization roadmap. The review feedback identifies several areas for improvement: addressing security risks of unencrypted HTTP downloads, adopting Ruff for linting and formatting, decoupling core logic from ML frameworks like scikit-learn, and including a CI/CD pipeline in the migration plan to automate testing.
| - **Lack of Automated Testing:** There is no test suite (e.g., `pytest`), making it risky to refactor or update dependencies. | ||
| - **Monolithic Design:** `heb_pipe.py` mixes CLI parsing, data downloading, pipeline orchestration, and formatting, making it difficult to import and use cleanly as a Python library. | ||
| - **Outdated Packaging:** Still relies on `setup.py` and a rigid `requirements.txt` instead of modern dependency management like `pyproject.toml` (Poetry/PDM). | ||
| - **Hardcoded Model Hosting:** Model weights are downloaded via hardcoded URLs from a personal domain (`gucorpling.org`), which is a single point of failure and not ideal for longevity. |
There was a problem hiding this comment.
The report correctly identifies hardcoded URLs as a single point of failure. However, it should also highlight that the current implementation uses unencrypted HTTP (http://gucorpling.org), which poses a significant security risk. Since model weights (often stored as pickle files) can execute arbitrary code upon loading, they should be served over HTTPS (e.g., via HuggingFace Hub) to prevent Man-in-the-Middle (MitM) attacks.
There was a problem hiding this comment.
Hello! I saw you tagged me. Is there anything specific you would like me to address regarding this line or the report in general?
|
|
||
| ## 4. Migration Plan | ||
| To rewrite and modernize this library from scratch, follow these phases: | ||
| 1. **Bootstrap Modern Project:** Initialize a new repository with `pyproject.toml`, `pytest`, `black`/`ruff` for formatting, and `mypy` for typing. |
There was a problem hiding this comment.
While black is mentioned, ruff is a more modern and performant alternative that handles formatting, linting, and import sorting. It is recommended to standardize on ruff to simplify the toolchain and improve developer experience.
1. Bootstrap Modern Project: Initialize a new repository with pyproject.toml, pytest, ruff for linting and formatting, and mypy for typing.
| To rewrite and modernize this library from scratch, follow these phases: | ||
| 1. **Bootstrap Modern Project:** Initialize a new repository with `pyproject.toml`, `pytest`, `black`/`ruff` for formatting, and `mypy` for typing. | ||
| 2. **Extract the Data Layer:** Move all the "Gold" data into a dedicated `data/` or `assets/` directory. Write clean data-loaders with `pydantic` models for structure. | ||
| 3. **Refactor Logic & Heuristics:** Port the core Hebrew-specific logic (from `preprocess.py`, `whitespace_tokenize.py`) into pure, testable functions with type hints. Write unit tests to guarantee 1:1 behavior with the legacy system. |
There was a problem hiding this comment.
The report identifies lib/preprocess.py as containing valuable rule-based logic. However, the current implementation is tightly coupled with scikit-learn (e.g., BaseEstimator, TransformerMixin). To achieve the goal of decoupling from ML frameworks, the migration plan should explicitly mention refactoring this logic into framework-agnostic Python functions.
3. Refactor Logic & Heuristics: Port the core Hebrew-specific logic (from preprocess.py, whitespace_tokenize.py) into pure, testable functions with type hints, removing dependencies on frameworks like scikit-learn. Write unit tests to guarantee 1:1 behavior with the legacy system.
| 2. **Extract the Data Layer:** Move all the "Gold" data into a dedicated `data/` or `assets/` directory. Write clean data-loaders with `pydantic` models for structure. | ||
| 3. **Refactor Logic & Heuristics:** Port the core Hebrew-specific logic (from `preprocess.py`, `whitespace_tokenize.py`) into pure, testable functions with type hints. Write unit tests to guarantee 1:1 behavior with the legacy system. | ||
| 4. **Decouple ML Models:** Replace the legacy `mtlmodel.py` with cleaner PyTorch Lightning modules or integrate natively with the HuggingFace `transformers` ecosystem. | ||
| 5. **Construct a Modular Pipeline:** Implement the pipeline using the Strategy pattern or HuggingFace Pipeline API, where tokenization, tagging, and parsing are distinct, pluggable components. |
There was a problem hiding this comment.
The migration plan identifies the lack of automated testing as a gap but does not explicitly include the setup of a Continuous Integration (CI) pipeline. Adding CI (e.g., GitHub Actions) is essential to ensure that tests and linters are run automatically on every contribution, maintaining the proposed modern standards.
5. Construct a Modular Pipeline & CI: Implement the pipeline using the Strategy pattern or HuggingFace Pipeline API, and set up CI/CD to automate testing and linting.
- Added `doc/report.txt` containing a detailed architecture and migration report. - The report covers project overview, data/logic analysis, gaps, migration plan, and recommendations for improvement. Co-authored-by: arielnahom <16294824+arielnahom@users.noreply.github.com>
Co-authored-by: arielnahom <16294824+arielnahom@users.noreply.github.com>
1 participant
This PR adds a comprehensive report outlining the current state of the HebPipe codebase and proposes a detailed re-architecture and migration plan to modernize the project.
Key points covered in the report (
doc/report.txt):PR created automatically by Jules for task 3353676423318959672 started by @arielnahom