Skip to content

Remove test_io_set_preload_false_is_faster#13933

Open
cbrnr wants to merge 2 commits into
mne-tools:mainfrom
cbrnr:remove-eeglab-preload-test
Open

Remove test_io_set_preload_false_is_faster#13933
cbrnr wants to merge 2 commits into
mne-tools:mainfrom
cbrnr:remove-eeglab-preload-test

Conversation

@cbrnr
Copy link
Copy Markdown
Contributor

@cbrnr cbrnr commented May 29, 2026

I suggest removing the test mne/io/eeglab/tests/test_eeglab.py::test_io_set_preload_false_is_faster for the following reasons:

  1. The correctness of not preloading is already tested in test_io_set_preload_false_uses_lazy_loading.
  2. A test relying on timing is by definition flaky (and it was also marked as such), which isn't good practice.
  3. The pytest.mark.flaky decorator without any args means reruns=1, which sometimes isn't enough.

This test just hit me in https://github.com/mne-tools/mne-python/actions/runs/26574036684/job/78288529573?pr=13680 and I really think it should be removed.

@cbrnr
Copy link
Copy Markdown
Contributor Author

cbrnr commented May 29, 2026

@larsoner since we're already discussing CirceCI in #13930, it is failing again because I'm not registered. Can you disable the "Organization Settings - Usage Controls - Prevent unregistered user spend" setting, which would fix this issue (see here)? Then contributors would not be forced to create an account with CircleCI (or use their GitHub account to grant permissions). This should be relatively safe, since this only concerns builds triggered via webhooks from GitHub (i.e., PRs).

@larsoner
Copy link
Copy Markdown
Member

Can you register instead (can just auth with GitHub)? CircleCI at one point suggested additional costs would be associated with unregistered users because they count as separate users (I think) so we'd like to keep that box ticked if we can

If you don't want to I can manually trigger the build instead

@cbrnr
Copy link
Copy Markdown
Contributor Author

cbrnr commented May 29, 2026

CircleCI at one point suggested additional costs would be associated with unregistered users because they count as separate users (I think) so we'd like to keep that box ticked if we can

Can we verify that this is actually the case? I don't think this is what the setting implies so it would be good to double-check.

In the meantime please start the job manually.

@larsoner
Copy link
Copy Markdown
Member

It's all a bit opaque... feel free to read https://support.circleci.com/hc/en-us/articles/360037744473-What-is-an-Unregistered-User and related docs if you're curious. In the meantime I looked at our CircleCI "Users" tab in usage and saw that we did have "Unregistered User" in the list for some months in 2025 (I can't remember when we disabled it). There aren't any users shown for recent months, I'm not sure if this is because we ticked the box, or if they've changed their accounting, or what. It's all a bit opaque.

I am hesitant to untick the box because based on what I've read a) there is a risk of incurring a financial cost (we pay for CircleCI) if we get it wrong, and b) it will create more maintenance work for me and/or Dan to make sure this isn't the case (monitoring, etc.). That being said, it's not off the table.

So far, contributors have been willing to click through the handful of boxes to sign up for CircleCI. But it sounds like you are strongly opposed to this @cbrnr ?

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.

2 participants