tests: rework downloading the wheelhouse for isolation tests #1199
Conversation
henryiii
force-pushed
the
henryiii/tests/moreoutputwheel
branch
from
9cca349 to
5737b86
Compare
Signed-off-by: Henry Schreiner <henryfs@princeton.edu>
henryiii
force-pushed
the
henryiii/tests/moreoutputwheel
branch
2 times, most recently
from
b3e2b1c to
4180ddf
Compare
Signed-off-by: Henry Schreiner <henryfs@princeton.edu>
henryiii
force-pushed
the
henryiii/tests/moreoutputwheel
branch
from
4180ddf to
f22883d
Compare
henryiii
changed the title
henryiii
force-pushed
the
henryiii/tests/moreoutputwheel
branch
from
4bb92ab to
3027ed9
Compare
Signed-off-by: Henry Schreiner <henryfs@princeton.edu>
henryiii
force-pushed
the
henryiii/tests/moreoutputwheel
branch
from
3027ed9 to
66af618
Compare
tests/conftest.py
Outdated
| check=True, | ||
| ) | ||
| return wheelhouse | ||
| if not all(list(wheelhouse.glob(f"{p}*.whl")) for p in download_wheels.WHEELS): |
There was a problem hiding this comment.
Btw, if you want to use uv.lock to guarantee some consistency and test more dependency drifts, feel free to do so. Just need an opt-in/out flag for building in the spec.
tests/conftest.py
Outdated
| @pytest.fixture(scope="session") | ||
| def pep518_wheelhouse(tmp_path_factory: pytest.TempPathFactory) -> Path: | ||
| wheelhouse = tmp_path_factory.mktemp("wheelhouse") | ||
| def pytest_configure(config) -> None: |
There was a problem hiding this comment.
Can you add an opt-out flag for this for building the rpm?
I checked again on how the building is done in fedora 1 and still no dice on making these work there.
One hing we could do is make the venv construct with --system-site-packages and skip all tests with build-isolation, but we would need a different interface method for installing dependencies that can be made no-op in that case.
Footnotes
There was a problem hiding this comment.
If we can put the wheels in here somehow, it should work, but if fedora has no internet access, we'll need to disable it. I believe, looking at it now, that if you disabled all "network" tests, then the fixture was never computed, which is why it was working before. Maybe we should use a filelock on the fixture instead.
There was a problem hiding this comment.
If we can put the wheels in here somehow, it should work, but if fedora has no internet access, we'll need to disable it
I need to push again about packaging the wheels, maybe even making a change proposal about it. But in the meantime disabling/auto-skipping the tests under there would be the most compatible approach right now. Having a pytest flag with pytest_addoption and skipping the wheelhouse setup should be enough in there since the tests are still being marked with network marker afaict
Edit: ah I see the file lock approach now
There was a problem hiding this comment.
Am I reading the failure in https://github.com/scikit-build/scikit-build-core/actions/runs/20453587799/job/58771162110?pr=1199 correctly that using the session scope on multiple workers some runs went ahead as if the session fixture was already setup? It would be replicable if we artificially add a ridiculous timeout under the wheels.lock. Wonder which of the 2 approaches would be better.
henryiii
force-pushed
the
henryiii/tests/moreoutputwheel
branch
from
8c41a23 to
e1c947f
Compare
| wheelhouse = pytestconfig.cache.mkdir("wheelhouse") | ||
|
|
||
| main_lock = FileLock(wheelhouse / "main.lock") | ||
| with main_lock: |
There was a problem hiding this comment.
I think you need to gate this by presence of scikit-build-core wheel.
Also don't think we need to 2 different locks?
There was a problem hiding this comment.
It's not an editable wheel, so it needs to be rebuilt every time the tests run - here it's being rebuilt by every process, but that's hopefully pretty cheap. Probably cheaper if we use build isolation.
There was a problem hiding this comment.
With two locks we can have one process start downloading wheels while the others are rebuilding the scikit-build-core wheel.
There was a problem hiding this comment.
It's not an editable wheel, so it needs to be rebuilt every time the tests run
Wait I don't get this part. Isn't it building the wheel so that every subsequent pip imstall is taking that wheel? I am thinking about the guarding for across the workers, but maybe you are considering it across different commits? It should be checked across the hatch-vcs version, which I think you were trying to do previously?
With two locks we can have one process start downloading wheels while the others are rebuilding the scikit-build-core wheel.
Good point, but that would only happen if it skips the locked functions gated by main.lock right? But speaking of which is it re-using the pip/uv cache? Using a uv.lock or equivalent could make the downloads be effectively no-ops
There was a problem hiding this comment.
It's testing the current source of scikit-build-core, so it has to be fresh. Yes, I'd like to only build it on one worker, but it's not that easy unless we make sure we always clean it up after a run (regardless of if the run failed, cancelled, etc).
There was a problem hiding this comment.
It's testing the current source of scikit-build-core, so it has to be fresh. Yes, I'd like to only build it on one worker, but it's not that easy unless we make sure we always clean it up after a run (regardless of if the run failed, cancelled, etc).
I did some local debugging hoping that hatch vcs could help with the dirty files, but it seems like that would not work since it doesn't check the actual contents of the files when it is dirty, only the current date and if there are any dirty files.
The only alternative I can think of is to explicitly check against the worker_id and only run it on master, but are we able to do that without hard-coding pytest-xdist dependency?
henryiii
force-pushed
the
henryiii/tests/moreoutputwheel
branch
2 times, most recently
from
b488388 to
a9acd43
Compare
Signed-off-by: Henry Schreiner <henryfs@princeton.edu>
henryiii
force-pushed
the
henryiii/tests/moreoutputwheel
branch
from
a9acd43 to
bbe56cf
Compare
|
The scikit-build-core/tests/plans.fmf Line 30 in 501ff0f |
Hopefully adding output capture will help us debug the intermittent failure on 3.12 • CMake 3.26 on windows-latest.
The problem is each worker is trying to download the same wheelhouse. I've reworked how the wheelhouse is setup, including a way to run the setup standalone so that something like Fedora could pre-download it before running pytest, no longer requiring the network.
python tests/utils/download_wheels.pyshould do it if pytest is normally configured. We could put these in a top-level folder if that's better.