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.

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

Don't use sub-venv for ocrd_segment #154

Closed
wants to merge 2 commits into from
Closed

Conversation

stweil
Copy link
Collaborator

@stweil stweil commented Aug 18, 2020

That processors don't use TensorFlow.

Signed-off-by: Stefan Weil sw@weilnetz.de

bertsky and others added 2 commits August 18, 2020 09:13
That processors don't use TensorFlow.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
@bertsky
Copy link
Collaborator

bertsky commented Aug 18, 2020

That processors don't use TensorFlow.

Oh, but they do (or will in the near future): look at the maskrcnn-cli branch... This is still work in progress, but I depend on this branch in other ocrd_all-based deployments.

@stweil
Copy link
Collaborator Author

stweil commented Aug 18, 2020

Oh, but they do (or will in the near future)

That's bad news. Maybe we should split such projects in two, one with processors which don't need a sub-venv, one with processors which need one? Or will all processors depend on TensorFlow in the near future?

Ideally we should not add new processors depending on old versions of TensorFlow ...

@bertsky
Copy link
Collaborator

bertsky commented Aug 18, 2020

That's bad news. Maybe we should split such projects in two, one with processors which don't need a sub-venv, one with processors which need one? Or will all processors depend on TensorFlow in the near future?

If you look at the README.md (outlining the original plan/idea for this repo), the TF-based MaskRCNN segmentation was always at the core of what we wanted to do here. The other (non-TF) processors were just preliminaries and fall-off which we exposed/exported early on, because they had value in the OCR-D ecosphere independently. I am reluctant to change that, because of the interdependencies. @wrznr what do you think?

Ideally we should not add new processors depending on old versions of TensorFlow ...

Agreed, but our MaskRCNN implementation has become unmaintained since we started using it, and it's not trivial to make the migration (there are already dozens of competing PRs targeting different TF2 versions in different ways). ocrd_anybaseocr is just one example of that effort (but they decided to just copy the source files into their own repo, so there's no point in follow them). Perhaps if I find some time to look at it, I'll merge and refine on of these PRs into my fork's master...

@bertsky
Copy link
Collaborator

bertsky commented Dec 17, 2020

BTW ocrd_segment's TF branch maskrcnn-cli will be based on TF2 soon, too. Closing for reasons explained above.

@bertsky bertsky closed this Dec 17, 2020
@stweil stweil deleted the ocrd_segment branch December 18, 2020 05:15
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.

None yet

2 participants