Skip to content

Conversation

kevinzwang
Copy link
Member

Changes Made

This test keeps failing in CI. Adding a retry here so that it succeeds.

Related Issues

Checklist

  • Documented in API Docs (if applicable)
  • Documented in User Guide (if applicable)
  • If adding a new documentation page, doc is added to docs/mkdocs.yml navigation
  • Documentation builds and is formatted properly (tag @/ccmao1130 for docs review)

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This PR fixes a flaky test in the transformers image embedding functionality by adding retry logic to handle transient failures during Hugging Face model instantiation. The change specifically targets the test_transformers_image_embedder_other test in tests/ai/test_transformers.py, which has been experiencing intermittent failures in CI environments.

The implementation adds a retry mechanism that attempts model instantiation up to 5 times with 5-second delays between attempts. The retry logic only catches OSError exceptions (which are typical for network/IO failures when downloading models) and re-raises the original exception if all retries are exhausted. A time import was added to support the sleep functionality between retry attempts.

This approach addresses the external dependency flakiness without modifying the core AI functionality. The test structure remains the same, with the retry logic wrapping only the problematic descriptor.instantiate() call. This is a pragmatic solution for dealing with transient external service issues that are outside the control of the Daft codebase, specifically related to Hugging Face's service reliability during model downloads.

Confidence score: 4/5

  • This PR is safe to merge with minimal risk as it only adds defensive retry logic to a flaky test
  • Score reflects a well-implemented retry mechanism with appropriate exception handling and reasonable retry parameters
  • No files require special attention as this is a straightforward test stability improvement

Context used:

Rule - Import statements should be placed at the top of the file rather than inline within functions or methods. (link)

1 file reviewed, no comments

Edit Code Review Bot Settings | Greptile

@desmondcheongzx
Copy link
Collaborator

I do wonder if we should slap the retries onto the instantiate itself instead of the test, but something for another time perhaps

@kevinzwang
Copy link
Member Author

@desmondcheongzx do you know why most of the tests are not running? I'm not super familiar with the skipcheck job which I assume is telling the other tests that it should be skipped.

@kevinzwang kevinzwang enabled auto-merge (squash) September 4, 2025 17:59
@kevinzwang kevinzwang merged commit 798dce8 into main Sep 4, 2025
41 checks passed
@kevinzwang kevinzwang deleted the kevin/flaky-embed-image-test branch September 4, 2025 18:39
Copy link

codecov bot commented Sep 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.88%. Comparing base (ef36735) to head (489f014).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5130   +/-   ##
=======================================
  Coverage   71.87%   71.88%           
=======================================
  Files         953      953           
  Lines      130522   130524    +2     
=======================================
+ Hits        93816    93821    +5     
+ Misses      36706    36703    -3     

see 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

venkateshdb pushed a commit to venkateshdb/Daft that referenced this pull request Sep 6, 2025
…5130)

## Changes Made

This test keeps failing in CI. Adding a retry here so that it succeeds.

## Related Issues

<!-- Link to related GitHub issues, e.g., "Closes Eventual-Inc#123" -->

## Checklist

- [ ] Documented in API Docs (if applicable)
- [ ] Documented in User Guide (if applicable)
- [ ] If adding a new documentation page, doc is added to
`docs/mkdocs.yml` navigation
- [ ] Documentation builds and is formatted properly (tag @/ccmao1130
for docs review)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants