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

fix: improve handling of disallowed formats #429

Merged
merged 6 commits into from
Dec 3, 2024
Merged

fix: improve handling of disallowed formats #429

merged 6 commits into from
Dec 3, 2024

Conversation

cau-git
Copy link
Contributor

@cau-git cau-git commented Nov 25, 2024 •
edited
Loading

Checklist:

  • Documentation has been updated, if necessary.
  • Examples have been added, if necessary.
  • Tests have been added, if necessary.

@cau-git cau-git mentioned this pull request Nov 25, 2024
Closed
@mergify Mergify
Copy link

mergify bot commented Nov 25, 2024

Merge Protections

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

🟢 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)(?:\(.+\))?:

vagenas
vagenas reviewed Nov 25, 2024
View reviewed changes
Copy link
Contributor

@vagenas vagenas left a comment

Choose a reason for hiding this comment

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

@cau-git

Regarding convert():
I see no need to touch the return type, we can just run next() in a try catch and in case of StopIteration raise a more user-friendly error instead (ideally some DoclingError we can define).

Regarding convert_all():
I would leave this untouched.

@dolfim-ibm
Copy link
Contributor

I also think we should not touch the return type, since the function have an explicit argument raises_on_error.

But it is still unclear to me what could/should be the return value when raises_on_error=False and the document is invalid?
Would a failed ConversionResult with the proper status, and the default _EMPTY_DOCLING_DOC be a good choice?

@vagenas
Copy link
Contributor

vagenas commented Nov 26, 2024

I also think we should not touch the return type, since the function have an explicit argument raises_on_error.

But it is still unclear to me what could/should be the return value when raises_on_error=False and the document is invalid? Would a failed ConversionResult with the proper status, and the default _EMPTY_DOCLING_DOC be a good choice?

Yes and we can introduce a new ConversionStatus.INVALID for that case.

cau-git and others added 4 commits November 26, 2024 21:03
@cau-git @vagenas
fix: Fixes and tests for StopIteration on .convert()
7df9527 
Signed-off-by: Christoph Auer <[email protected]>
@cau-git @vagenas
fix: Remove unnecessary case handling
c8bf252 
Signed-off-by: Christoph Auer <[email protected]>
@cau-git @vagenas
fix: Other test fixes
bafe673 
Signed-off-by: Christoph Auer <[email protected]>
@vagenas
improve handling of unsupported types
0bb1e20 
- Introduced new explicit exception types instead of `RuntimeError`
- Introduced new `ConversionStatus` value for unsupported formats
- Tidied up converter member typing & removed asserts

Signed-off-by: Panos Vagenas <[email protected]>
@vagenas vagenas force-pushed the cau/fix-stop-iteration branch from 495503f to 0bb1e20 Compare November 26, 2024 20:33
PeterStaar-IBM
PeterStaar-IBM reviewed Nov 28, 2024
View reviewed changes
docling/datamodel/base_models.py Outdated
@@ -22,6 +22,7 @@ class ConversionStatus(str, Enum):
FAILURE = auto()
SUCCESS = auto()
PARTIAL_SUCCESS = auto()
UNSUPPORTED = auto()
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this UNSUPPORTED_FORMAT to be a bit more explicit

PeterStaar-IBM
PeterStaar-IBM requested changes Nov 28, 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.

Let's make UNSUPPORTED into UNSUPPORTED_FORMAT to be a bit more explicit. All the rest is good!

@cau-git
Copy link
Contributor Author

cau-git commented Dec 2, 2024 •
edited
Loading

We must agree on the naming, instead of UNSUPPORTED.

DROPPED
DISCARDED
SKIPPED # favourite

and add a reason that explains properly what went wrong as an ErrorItem in the errors log of ConversionResult.

@vagenas vagenas changed the title fix: Fixes and tests for StopIteration on .convert() fix: improve handling of disallowed formats Dec 2, 2024
@cau-git cau-git merged commit 34c7c79 into main Dec 3, 2024
9 checks passed
@cau-git cau-git deleted the cau/fix-stop-iteration branch December 3, 2024 11:45
ab-shrek pushed a commit to ab-shrek/docling that referenced this pull request Dec 6, 2024
@cau-git @vagenas @ab-shrek
fix: improve handling of disallowed formats (DS4SD#429)
a1690a0 
* fix: Fixes and tests for StopIteration on .convert()

Signed-off-by: Christoph Auer <[email protected]>

* fix: Remove unnecessary case handling

Signed-off-by: Christoph Auer <[email protected]>

* fix: Other test fixes

Signed-off-by: Christoph Auer <[email protected]>

* improve handling of unsupported types

- Introduced new explicit exception types instead of `RuntimeError`
- Introduced new `ConversionStatus` value for unsupported formats
- Tidied up converter member typing & removed asserts

Signed-off-by: Panos Vagenas <[email protected]>

* robustify & simplify format option resolution

Signed-off-by: Panos Vagenas <[email protected]>

* rename new status, populate ConversionResult errors

Signed-off-by: Panos Vagenas <[email protected]>

---------

Signed-off-by: Christoph Auer <[email protected]>
Signed-off-by: Panos Vagenas <[email protected]>
Co-authored-by: Panos Vagenas <[email protected]>
lucas-morin pushed a commit to lucas-morin/docling that referenced this pull request Dec 10, 2024
@cau-git @vagenas @lucas-morin
fix: improve handling of disallowed formats (DS4SD#429)
319a7ef 
* fix: Fixes and tests for StopIteration on .convert()

Signed-off-by: Christoph Auer <[email protected]>

* fix: Remove unnecessary case handling

Signed-off-by: Christoph Auer <[email protected]>

* fix: Other test fixes

Signed-off-by: Christoph Auer <[email protected]>

* improve handling of unsupported types

- Introduced new explicit exception types instead of `RuntimeError`
- Introduced new `ConversionStatus` value for unsupported formats
- Tidied up converter member typing & removed asserts

Signed-off-by: Panos Vagenas <[email protected]>

* robustify & simplify format option resolution

Signed-off-by: Panos Vagenas <[email protected]>

* rename new status, populate ConversionResult errors

Signed-off-by: Panos Vagenas <[email protected]>

---------

Signed-off-by: Christoph Auer <[email protected]>
Signed-off-by: Panos Vagenas <[email protected]>
Co-authored-by: Panos Vagenas <[email protected]>
cau-git added a commit that referenced this pull request Dec 17, 2024
@cau-git @vagenas
fix: improve handling of disallowed formats (#429)
0be7362 
* fix: Fixes and tests for StopIteration on .convert()

Signed-off-by: Christoph Auer <[email protected]>

* fix: Remove unnecessary case handling

Signed-off-by: Christoph Auer <[email protected]>

* fix: Other test fixes

Signed-off-by: Christoph Auer <[email protected]>

* improve handling of unsupported types

- Introduced new explicit exception types instead of `RuntimeError`
- Introduced new `ConversionStatus` value for unsupported formats
- Tidied up converter member typing & removed asserts

Signed-off-by: Panos Vagenas <[email protected]>

* robustify & simplify format option resolution

Signed-off-by: Panos Vagenas <[email protected]>

* rename new status, populate ConversionResult errors

Signed-off-by: Panos Vagenas <[email protected]>

---------

Signed-off-by: Christoph Auer <[email protected]>
Signed-off-by: Panos Vagenas <[email protected]>
Co-authored-by: Panos Vagenas <[email protected]>
Signed-off-by: Christoph Auer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vagenas vagenas vagenas approved these changes

@PeterStaar-IBM PeterStaar-IBM PeterStaar-IBM approved these changes

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

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

Successfully merging this pull request may close these issues.
4 participants
@cau-git @dolfim-ibm @vagenas @PeterStaar-IBM