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

process validation: allow tasks without outfilegrp #296

Merged
merged 12 commits into from
Nov 15, 2019
Merged

process validation: allow tasks without outfilegrp #296

merged 12 commits into from
Nov 15, 2019

Conversation

bertsky
Copy link
Collaborator

@bertsky bertsky commented Aug 29, 2019

Fixes #295

@bertsky bertsky requested a review from kba August 29, 2019 09:06
@bertsky
Copy link
Collaborator Author

bertsky commented Aug 29, 2019

Obviously I forgot to adapt the validator test. So do we skip the offending test_parse_no_out altogether, or is there anything we could test instead?

@bertsky
Copy link
Collaborator Author

bertsky commented Aug 29, 2019

Maybe the validator can be made sensitive of whether the task (as per ocrd-tool) should actually have an output file group or not?

kba added a commit to kba/spec that referenced this pull request Sep 3, 2019
@kba
🔥 --output-file-grp optional, OCR-D/core#296
b719364
@bertsky bertsky mentioned this pull request Sep 6, 2019
Merged
@kba kba mentioned this pull request Sep 6, 2019
Closed
kba added a commit to kba/ocrd-core that referenced this pull request Sep 11, 2019
@kba
[WIP] make ProcessorTasks aware of the ocrd-tool.json of the processo…
6b8af8b 
…r they are wrapping

c.f. OCR-D#295 OCR-D#296
kba added a commit to kba/spec that referenced this pull request Oct 23, 2019
@kba
ocrd-tool: output_file_grp is not required
b9a49d2 
OCR-D/core#296
OCR-D#125
kba added a commit to kba/spec that referenced this pull request Oct 23, 2019
@kba
ocrd-tool: output_file_grp is not required
30b73c7 
OCR-D/core#296
OCR-D#125
kba added a commit to OCR-D/spec that referenced this pull request Oct 23, 2019
@kba
Merge pull request #125 from kba/output-filegrp-optional
3d6c8a4 
🔥 --output-file-grp optional, OCR-D/core#296
@codecov-io
Copy link

codecov-io commented Oct 24, 2019 •
edited
Loading

Codecov Report

Merging #296 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #296      +/-   ##
==========================================
- Coverage   90.72%   90.71%   -0.01%     
==========================================
  Files          30       30              
  Lines        1649     1648       -1     
  Branches      319      320       +1     
==========================================
- Hits         1496     1495       -1     
  Misses        112      112              
  Partials       41       41
Impacted Files Coverage Δ
ocrd/ocrd/decorators.py 79.48% <ø> (-5.97%) ⬇️
ocrd_utils/ocrd_utils/__init__.py 83.25% <100%> (+1.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6097c3...efb6fcb. Read the comment docs.

bertsky
bertsky commented Oct 25, 2019
View reviewed changes
Copy link
Collaborator Author

@bertsky bertsky left a comment

Choose a reason for hiding this comment

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

This is great, thank you!

Just wondering: as more stuff leaves the decorators, I hope you don't forget that we need all the process (JSON, file grps) validation in the decorator already. See #251. Will that still be possible?

@codecov-io
Copy link

codecov-io commented Oct 25, 2019 •
edited
Loading

Codecov Report

Merging #296 into master will not change coverage.
The diff coverage is 94.73%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #296   +/-   ##
=======================================
  Coverage   85.32%   85.32%           
=======================================
  Files          30       30           
  Lines        1758     1758           
  Branches      339      340    +1     
=======================================
  Hits         1500     1500           
  Misses        208      208           
  Partials       50       50
Impacted Files Coverage Δ
ocrd/ocrd/decorators.py 79.48% <ø> (-5.97%) ⬇️
ocrd_utils/ocrd_utils/__init__.py 66.53% <94.73%> (+2.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14ed1af...18d081b. Read the comment docs.

@cneud cneud self-requested a review November 4, 2019 17:26
cneud
cneud approved these changes Nov 4, 2019
View reviewed changes
Copy link
Member

@cneud cneud left a comment

Choose a reason for hiding this comment

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

OK to merge but we also need to adapt spec accordingly

@kba
Copy link
Member

kba commented Nov 12, 2019

we also need to adapt spec accordingly

Already adapted, output_file_grp is optional in ocrd-tool.json and CLI spec lists parameter as optional.

@kba kba merged commit 32c46a8 into master Nov 15, 2019
@kba kba deleted the allow-non-output-tasks branch November 15, 2019 15:24
bertsky pushed a commit to bertsky/core that referenced this pull request Apr 26, 2023
@stweil
Merge pull request OCR-D#296 from stweil/detectron2
ceef6cf 
Update ocrd_detectron2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cneud cneud cneud approved these changes

@kba kba Awaiting requested review from kba

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

Successfully merging this pull request may close these issues.

ocrd process: allow non-output tasks
4 participants
@bertsky @codecov-io @kba @cneud