fix: add missing requires_gpu and requires_heavy_ram markers to Huggi…#621
fix: add missing requires_gpu and requires_heavy_ram markers to Huggi…#621planetf1 wants to merge 1 commit into
Conversation
…ngFace metrics test test_huggingface_token_metrics_integration was missing the markers that trigger conftest auto-skip logic on systems without sufficient GPU or RAM (threshold: 48GB). Without them the test runs unconditionally, loading a full HF model and consuming excessive time and memory. Fixes generative-computing#620
|
The PR description has been updated. Please fill out the template for your PR to be reviewed. |
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
ajbozarth
left a comment
There was a problem hiding this comment.
Details below, but I don't think this is a necessary fix. If it's failing for you (or more people) we should identify why because it may be a different issue.
| @pytest.mark.skipif( | ||
| int(os.environ.get("CICD", 0)) == 1, | ||
| reason="Skipping HuggingFace metrics test in CI - requires GPU and model download", | ||
| ) |
There was a problem hiding this comment.
this is handled below by
if gh_run:
pytest.skip("Skipping in CI - requires model download")
Should we replace that code with this decorator? Is the decorator the new standard way to skip in CI?
| @pytest.mark.requires_gpu | ||
| @pytest.mark.requires_heavy_ram |
There was a problem hiding this comment.
I considered adding these, but in all my testing they didn't seem to apply. I saw these regularly take 30sec per run as long as the required model was already downloaded. I also made sure to choose a model that was used in other tests so these tests wouldn't download a new model only to use here
|
So I tested this and found that while the tests behave as I expect when just running the telemetry tests, that they do hang as @planetf1 found when running the entire suite. After trying out #621 to see if it addressed the problem I found that it will actually make it worse without addressing this. But instead of just skipping I found that the issue was without the test isolation used in other hf tests (due to their need of high ram/gpu) that these tests were loading the model into memory multiple times and not clearing it. I have a better fix that adds a fixture to address this that I'll open in a bit after some more testing |
|
Thanks @ajbozarth your fix is more appropriate. Closing. |
Misc PR
Type of PR
Description
test_huggingface_token_metrics_integrationwas added in #563 withoutrequires_gpuorrequires_heavy_rammarkers. All other HuggingFace tests carry both markers, which are enforced byconftest.pyat collection time —requires_heavy_ramskips on systems with < 48 GB RAM,requires_gpuskips without a GPU. Without them the test ran unconditionally on any machine with OpenTelemetry installed, triggering a full model download and load that consumed 10–15 minutes and exhausted available memory.On my macbook m1 32GB this stalled the system for 15 mins before i aborted it, very high memory pressure (>40GB) led to audio breakup/stuttering, and finally had to be terminated.
Testing