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

Increase MAX_IMPORT_TIME_MS in tests up to 800ms #1018

Merged
merged 1 commit into from
Apr 10, 2025

Conversation

dreadatour
Copy link
Contributor

@dreadatour dreadatour commented Apr 1, 2025

This test fails too often :(

Latest example: https://github.com/iterative/datachain/actions/runs/14176251298/job/39711770663

Will be happy to have better option and also will be happy to discuss this.

@dreadatour dreadatour requested a review from skshetry April 1, 2025 14:35
@dreadatour dreadatour self-assigned this Apr 1, 2025
Copy link

Deploying datachain-documentation with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3a679d6
Status: ✅  Deploy successful!
Preview URL: https://bf7ef275.datachain-documentation.pages.dev
Branch Preview URL: https://increase-max-import-time-tes.datachain-documentation.pages.dev

View logs

Copy link

codecov bot commented Apr 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.13%. Comparing base (ce38785) to head (3a679d6).
Report is 7 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1018   +/-   ##
=======================================
  Coverage   88.13%   88.13%           
=======================================
  Files         145      145           
  Lines       12355    12355           
  Branches     1713     1713           
=======================================
  Hits        10889    10889           
  Misses       1048     1048           
  Partials      418      418           
Flag Coverage Δ
datachain 88.06% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@shcheklein
Copy link
Member

@dreadatour why is it happening? is there a chance that we actually making it worse gradually and need to do a research again?

@dreadatour
Copy link
Contributor Author

@dreadatour why is it happening? is there a chance that we actually making it worse gradually and need to do a research again?

It is possible, but I have seen this test fails before, many times, and rerun helps. I think this is because of GitHub CI runners, but I am not sure, that's why I have mentioned @skshetry in this PR, I would like to discuss this issue with him before merging this.

We also have this discussion before, in this PR: #974

@ilongin
Copy link
Contributor

ilongin commented Apr 1, 2025

What is the max import time we are aiming for? ... Anyway, I would rather skip the test for now and create an issue to fix it, then increasing import time but I don't have strong opinion.
It makes sense that it's because of Github workers and running tests in parallel as you mentioned in related issue.

@dreadatour
Copy link
Contributor Author

What is the max import time we are aiming for?

from @skshetry (here):

It was taking ~450ms on ubuntu-latest-8-cores last I checked, so I added some slack and set it to 700ms. It should never reach that number, much less exceed it

@dreadatour
Copy link
Contributor Author

@skshetry it would be great if we can discuss this 🙏

The test failed here too, as well: https://github.com/iterative/datachain/actions/runs/14367963543/job/40298766582?pr=1035

FAILED tests/test_import_time.py::test_import_time - AssertionError: Possible import time regression; took 757ms
assert 757 < 700

@skshetry
Copy link
Member

What is the max import time we are aiming for?

I'd like it to be <100ms, but we are pretty far away from that.

@dreadatour dreadatour merged commit a337965 into main Apr 10, 2025
35 checks passed
@dreadatour dreadatour deleted the increase-max-import-time-test branch April 10, 2025 11:02
@dreadatour
Copy link
Contributor Author

What is the max import time we are aiming for?

I'd like it to be <100ms, but we are pretty far away from that.

I know 😥 We'll be there, one day 🤗

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.

4 participants