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

test(python,rust): Refactor failing test #9823

Merged
merged 4 commits into from
Jul 12, 2023
Merged

test(python,rust): Refactor failing test #9823

merged 4 commits into from
Jul 12, 2023

Conversation

stinodego
Copy link
Member

@stinodego stinodego commented Jul 12, 2023

Related to #9122

Changes:

@github-actions github-actions bot added ci Related to the continuous integration setup python Related to Python Polars rust Related to Rust Polars labels Jul 12, 2023
@stinodego stinodego changed the title ci(python,rust): Skip failing test test(python,rust): Refactor failing test Jul 12, 2023
@stinodego stinodego removed the ci Related to the continuous integration setup label Jul 12, 2023
@github-actions github-actions bot added the test Related to the test suite label Jul 12, 2023
@stinodego stinodego marked this pull request as draft July 12, 2023 06:52
@stinodego stinodego force-pushed the skip-failing-test branch 2 times, most recently from e5ea614 to 4cacf4d Compare July 12, 2023 07:10
@stinodego stinodego marked this pull request as ready for review July 12, 2023 07:23
@ritchie46
Copy link
Member

Out of curiousity? How does that solve the failing test? Was there a race condition?

@stinodego
Copy link
Member Author

Out of curiousity? How does that solve the failing test? Was there a race condition?

Not sure if this will fix it. But generally running multiple tests in one test function makes things a bit more unstable. While this might not help, at least the test will be factored a bit better.

What we really need is some way to control where the disk spill happens, and make sure things are written to a properly controlled temporary directory. Can we configure disk spill somehow?

@ritchie46
Copy link
Member

I think it can be race conditions now that I think of it. The ooc sort/groupby runs a garbage collection job that deletes previous runs. Can we force those tests to not run in parallel?

@stinodego
Copy link
Member Author

I think it can be race conditions now that I think of it. The ooc sort/groupby runs a garbage collection job that deletes previous runs. Can we force those tests to not run in parallel?

Right, so one runner might delete data on disk that a test on another runner relies on.

Sure, we can have all streaming tests on one runner. Let me figure out how to configure that nicely.

@stinodego stinodego force-pushed the skip-failing-test branch from 4cacf4d to 95fe580 Compare July 12, 2023 10:45
@stinodego
Copy link
Member Author

This should do it. Place your OOC tests in the test_ooc.py module and they should all run on the same worker. Or if your test really must live somewhere else, add the pytest marker @pytest.mark.xdist_group("ooc").

@stinodego stinodego marked this pull request as draft July 12, 2023 10:54
@stinodego
Copy link
Member Author

@ritchie46 The tests now run sequentially on a single worker, but it only made things worse 😕 now other tests are failing as well.

Looks like there might be a deeper issue with the garbage collection, or something else that causes subsequent streaming operations to fail?

@ritchie46
Copy link
Member

Alright, let's leave it for now. I can take a look at this later.

@stinodego stinodego force-pushed the skip-failing-test branch from 95fe580 to 9701c55 Compare July 12, 2023 11:33
@stinodego
Copy link
Member Author

stinodego commented Jul 12, 2023

Sure. I'll merge the initial refactor part, we can address the underlying issues later - I'll open a draft PR for that.

@stinodego stinodego marked this pull request as ready for review July 12, 2023 11:56
@stinodego stinodego merged commit 6812c65 into main Jul 12, 2023
@stinodego stinodego deleted the skip-failing-test branch July 12, 2023 11:56
c-peters pushed a commit to c-peters/polars that referenced this pull request Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Related to Python Polars rust Related to Rust Polars test Related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants