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.

Sign up for GitHub

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

Jump to bottom

feat: add support for ocrmac OCR engine on macOS #276

Merged
merged 7 commits into from
Nov 20, 2024
Merged

feat: add support for ocrmac OCR engine on macOS #276

merged 7 commits into from
Nov 20, 2024

Conversation

nuridol
Copy link
Contributor

@nuridol nuridol commented Nov 8, 2024

  • Integrates ocrmac as an OCR engine option for macOS users.
  • Adds configuration options and dependencies for ocrmac.
  • Updates documentation to reflect new engine support.

This change allows macOS users to utilize ocrmac for improved OCR performance and compatibility.

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the
    conventional commits.
  • Documentation has been updated, if necessary.
  • Examples have been added, if necessary.
  • Tests have been added, if necessary.

@PeterStaar-IBM
Copy link
Contributor

@nuridol This looks amazing, thank you for the PR! I enabled the test. It seems you need to run

poetry lock

Let's iterate!

nuridol and others added 2 commits November 8, 2024 15:12
@nuridol
feat: add support for ocrmac OCR engine on macOS
4aaf128 
- Integrates `ocrmac` as an OCR engine option for macOS users.
- Adds configuration options and dependencies for `ocrmac`.
- Updates documentation to reflect new engine support.

This change allows macOS users to utilize `ocrmac` for improved OCR performance and compatibility.

Signed-off-by: Suhwan Seo <[email protected]>
updated the poetry lock
719cfe9 
Signed-off-by: Suhwan Seo <[email protected]>
@nuridol
Copy link
Contributor Author

nuridol commented Nov 8, 2024

@nuridol This looks amazing, thank you for the PR! I enabled the test. It seems you need to run

poetry lock

Let's iterate!

Thank you for your feedback! I've committed the updates you suggested.

@PeterStaar-IBM
Copy link
Contributor

Let me keep running the tests, and let's see what comes out. We have a code-freeze by Nov 15th for productization, so I wont merge it before that, to ensure that other dependencies come in. However, it looks like a very nice addition.

just to confirm:

  1. it is ocrmac with MIT License (https://pypi.org/project/ocrmac/)?
  2. Can you make sure that the package supports python 3.10, 3.11, 3.12 and 3.13. The last is not yet a strong requirement, but it will once torch supports 3.13.

@PeterStaar-IBM
Copy link
Contributor

PeterStaar-IBM commented Nov 8, 2024 •
edited
Loading

@nuridol Next to do, run the formatting (multiple times),

poetry run pre-commit run --all-files

You should see this, after a few repeated times running the command,

taa@Munlochy docling % poetry run pre-commit run --all-files
Black....................................................................Passed
isort....................................................................Passed
MyPy.....................................................................Passed
nbQA Black...............................................................Passed
nbQA isort...............................................................Passed
Poetry check.............................................................Passed

@PeterStaar-IBM
Copy link
Contributor

I dont see any updates on the tests, did you see the ocr-tests we have (https://github.com/DS4SD/docling/blob/main/tests/test_e2e_ocr_conversion.py)?

Fix linting issues, update CLI docs, and add error for ocrmac use on …
944988c 
…non-Mac systems

- Resolved formatting and linting issues
- Updated `--ocr-engine` CLI option documentation for `ocrmac`
- Added RuntimeError for attempts to use `ocrmac` on non-Mac platforms

Signed-off-by: Suhwan Seo <[email protected]>
@nuridol
Copy link
Contributor Author

nuridol commented Nov 8, 2024 •
edited
Loading

Hi, apologies for the multiple back-and-forth. Thanks again for considering this a valuable addition, and I understand the plan regarding the code freeze.

Here’s an update:

  1. I’ve confirmed that ocrmac indeed operates under the MIT License, as indicated in the repository.
  2. I’ve successfully tested the package in local environments for Python 3.10 through 3.12, with conversions running smoothly. I couldn’t test 3.13 due to the torch version incompatibility blocking docling installation, but ocrmac and the other code appear compatible.
  3. I ran both poetry run pre-commit run --all-files and pytest commands multiple times and can confirm that all checks passed.
  4. I also corrected an error where ocrmac would produce issues when not run in a Mac environment.

Thanks, and I look forward to the next steps!

@dolfim-ibm
Copy link
Contributor

@nuridol we are ready to finalize this work. Would you have some time to rebase the current branch with the latest main?

nuridol and others added 2 commits November 20, 2024 18:20
@nuridol
feat: add support for ocrmac OCR engine on macOS
0c23b69 
- Integrates `ocrmac` as an OCR engine option for macOS users.
- Adds configuration options and dependencies for `ocrmac`.
- Updates documentation to reflect new engine support.

This change allows macOS users to utilize `ocrmac` for improved OCR performance and compatibility.

Signed-off-by: Suhwan Seo <[email protected]>
docs: update examples and installation for ocrmac support
2576e65 
- Added `OcrMacOptions` to `custom_convert.py` and `full_page_ocr.py` examples.
- Included usage comments and examples for `OcrMacOptions` in OCR pipelines.
- Updated installation guide to include instructions for installing `ocrmac`, noting macOS version requirements (10.15+).
- Highlighted that `ocrmac` leverages Apple's Vision framework as an OCR backend.

This enhances documentation for users working on macOS to leverage `ocrmac` effectively.

Signed-off-by: Suhwan Seo <[email protected]>
@mergify Mergify
Copy link

mergify bot commented Nov 20, 2024 •
edited
Loading

Merge Protections

Your pull request matches the following merge protections and will not be merged until they are valid.

🟢 🛡 GitHub branch protections and repository rulesets requirements

Wonderful, this rule succeeded.
  • #approved-reviews-by >= 1 [🛡 GitHub repository ruleset rule main]
  • #changes-requested-reviews-by = 0 [🛡 GitHub repository ruleset rule main]
  • any of: [🛡 GitHub repository ruleset rule main]
    • check-success = code-checks / run-checks (3.10)
    • check-neutral = code-checks / run-checks (3.10)
    • check-skipped = code-checks / run-checks (3.10)
  • any of: [🛡 GitHub repository ruleset rule main]
    • check-success = code-checks / run-checks (3.11)
    • check-neutral = code-checks / run-checks (3.11)
    • check-skipped = code-checks / run-checks (3.11)
  • any of: [🛡 GitHub repository ruleset rule main]
    • check-success = code-checks / run-checks (3.12)
    • check-neutral = code-checks / run-checks (3.12)
    • check-skipped = code-checks / run-checks (3.12)

🟢 Enforce conventional commit

Wonderful, this rule succeeded.

Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/

  • title ~= ^(fix|feat|docs|style|refactor|perf|test|build|ci|chore|revert)(?:\(.+\))?:

PeterStaar-IBM
PeterStaar-IBM previously approved these changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@PeterStaar-IBM PeterStaar-IBM left a comment

Choose a reason for hiding this comment

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

Wonderful!

dolfim-ibm
dolfim-ibm requested changes Nov 20, 2024
View reviewed changes
@nuridol
Copy link
Contributor Author

nuridol commented Nov 20, 2024

@nuridol we are ready to finalize this work. Would you have some time to rebase the current branch with the latest main?

I've updated the branch by rebasing it with the latest main.
Thank you, and I look forward to your feedback!

feat: add optional ocrmac support
c13b128 
- Added `ocrmac` as an optional dependency in `pyproject.toml` and `poetry.lock`.
- Updated the `[tool.poetry.extras]` section to include `ocrmac`.
- Modified end-to-end OCR conversion tests to support `OcrMacOptions` on macOS.

Signed-off-by: Suhwan Seo <[email protected]>
dolfim-ibm
dolfim-ibm requested changes Nov 20, 2024
View reviewed changes
fix: update ocrmac dependency with macOS-specific marker
a277ea6 
- Added `sys_platform == 'darwin'` marker to the `ocrmac` dependency in `pyproject.toml` to specify macOS compatibility.
- Updated the content hash in `poetry.lock` to reflect the changes.

This ensures the `ocrmac` dependency is only installed on macOS systems.

Signed-off-by: Suhwan Seo <[email protected]>
@dolfim-ibm dolfim-ibm merged commit 6efa96c into DS4SD:main Nov 20, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dolfim-ibm dolfim-ibm dolfim-ibm approved these changes

@PeterStaar-IBM PeterStaar-IBM PeterStaar-IBM left review comments

@cau-git cau-git Awaiting requested review from cau-git

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nuridol @PeterStaar-IBM @dolfim-ibm