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

Fix a failing benchmark test #290

Merged
merged 4 commits into from
Oct 8, 2024
Merged

Fix a failing benchmark test #290

merged 4 commits into from
Oct 8, 2024

Conversation

jonchang
Copy link
Collaborator

@jonchang jonchang commented Oct 7, 2024

Description

CI is hitting this test error:

    def image_to_text(self, segments: dict[str, np.ndarray]) -> dict[str, tuple[str, float]]:
        digitized: dict[str, tuple[str, float]] = {}
        for label, image in segments.items():
            if image is None:
                continue

            generated_text = []
            confidence = []

            text_blocks = list(self.split_text_blocks(image))

            # Ignore output from `split_text_blocks` algorithm if only one text block is detected
            if len(text_blocks) == 1:
                text_blocks = [image]

            for block in text_blocks:
                pixel_values = self.processor(images=block, return_tensors="pt").pixel_values
                with torch.no_grad():
                    outputs = self.model.generate(
                        pixel_values, max_new_tokens=100, output_scores=True, return_dict_in_generate=True
                    )

                generated_text.append(self.processor.batch_decode(outputs.sequences, skip_special_tokens=True)[0])
                confidence.append(self.calculate_confidence(outputs))

>           digitized[label] = (" ".join(generated_text), min(confidence))
E           ValueError: min() iterable argument is empty

This pull request fixes the failing test by widening the check against the split text blocks algorithm's result.

Screenshots (if applicable)

Related Issues

Follow up to #248

Checklist

  • The title of this PR is descriptive and concise.
  • My changes follow the style guidelines of this project.
  • I have added or updated test cases to cover my changes.
  • I've let the team know about this PR by linking it in the review channel

Copy link
Collaborator

@arinkulshi-skylight arinkulshi-skylight left a comment

Choose a reason for hiding this comment

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

LG

Copy link
Collaborator

@schreiaj schreiaj left a comment

Choose a reason for hiding this comment

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

If it's not a huge hassle, could you build a test case that exercises this distinction? Would help prevent potential regressions down the road when we lose the context behind why it's <= 1 instead of == 1

@jonchang
Copy link
Collaborator Author

jonchang commented Oct 7, 2024

I think part of the issue is that split_text_blocks could potentially yield no result. This wasn't something that I expected but the test exposed it since it is passing in extremely small images that can't really be split

@jonchang jonchang requested a review from schreiaj October 8, 2024 15:57
@jonchang jonchang force-pushed the fix-failing-benchmark-test branch from c6bc25a to 9708325 Compare October 8, 2024 16:06
@jonchang jonchang added this pull request to the merge queue Oct 8, 2024
Merged via the queue into main with commit d639651 Oct 8, 2024
2 checks passed
@jonchang jonchang deleted the fix-failing-benchmark-test branch October 8, 2024 18:35
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.

3 participants