Skip to content

fix: keep GPU fingerprint import safe on CPU CI#6818

Merged
Scottcjn merged 2 commits into
Scottcjn:mainfrom
yyswhsccc:bounty-radar/gpu-fingerprint-import-safe
Jun 3, 2026
Merged

fix: keep GPU fingerprint import safe on CPU CI#6818
Scottcjn merged 2 commits into
Scottcjn:mainfrom
yyswhsccc:bounty-radar/gpu-fingerprint-import-safe

Conversation

@yyswhsccc

Copy link
Copy Markdown
Contributor

Summary

  • Defers PyTorch/CUDA requirement checks until the GPU fingerprint flow actually runs.
  • Keeps module import safe on CPU-only CI / no-torch environments.
  • Adds an import-only regression test for the no-torch path.

Follow-up scope

This is the small GPU import-safety slice called out as acceptable in #6711.

It intentionally does not touch node/admin/auth endpoints, production security guards, or the repo-wide stale-test/admin-key baseline handled separately in #6788.

Validation

  • python3 -m py_compile miners/gpu_fingerprint.py tests/test_gpu_fingerprint_import.py
  • uv run --no-project --with pytest --with flask --with requests --with flask-cors --with httpx --with cryptography python -m pytest -q tests/test_gpu_fingerprint_import.py --tb=short -o addopts='' --basetemp=.pytest-tmp -> 1 passed
  • git diff --check
  • python3 miners/gpu_fingerprint.py --json without torch -> exits 1 with ERROR: PyTorch with CUDA support required. Install: pip install torch

Refs #305.
Follow-up to #6711.

wallet: RTC47bc28896a1a4bf240d1fd780f4559b242bcd945

@github-actions github-actions Bot added BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) tests Test suite changes size/M PR: 51-200 lines labels Jun 3, 2026

@vinhcafe25-ops vinhcafe25-ops left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: PR #6818 — fix: keep GPU fingerprint import safe on CPU CI

Tier: Thorough — Line-by-line review. Core approach is sound; the test has a critical bug.


Finding 1 (🔴 Bug — Test): monkeypatch.setitem(sys.modules, "torch", None) does NOT simulate a missing torch

File: tests/test_gpu_fingerprint_import.py

monkeypatch.setitem(sys.modules, "torch", None)
monkeypatch.setitem(sys.modules, "torch.cuda", None)

This does not cause import torch to raise ImportError. Python's import machinery checks sys.modules via key existence (name in sys.modules), not truthiness. Setting the entry to None means:

  1. import torch → finds key in sys.modules → returns Noneno ImportError
  2. import torch.cuda → finds key in sys.modules → returns Noneno ImportError
  3. The except ImportError block is never reached
  4. The else branch runs → HAS_TORCH = True
  5. assert module.HAS_TORCH is FalseFAILS

The test should delete the keys from sys.modules instead, which is what actually happens on a machine without torch:

# Correct approach: remove from sys.modules so import raises ImportError
monkeypatch.delitem(sys.modules, "torch", raising=False)
monkeypatch.delitem(sys.modules, "torch.cuda", raising=False)

Or use importlib mocking to make the import genuinely raise:

import builtins
original_import = builtins.__import__
def mock_import(name, *args, **kwargs):
    if name in ("torch", "torch.cuda"):
        raise ImportError(f"No module named '{name}'")
    return original_import(name, *args, **kwargs)
monkeypatch.setattr(builtins, "__import__", mock_import)

Impact: The test either never actually ran, or it "passes" by coincidence (e.g., torch was already imported in the test session and monkeypatch.setitem overwrites the real module, but the spec.loader.exec_module path might bypass sys.modules in some edge case). Either way, the test is not testing what it claims to test.


Finding 2 (🟡 Fragility): Both torch and torch.cuda share one except block

File: miners/gpu_fingerprint.py

try:
    import torch
    import torch.cuda
except ImportError:
    torch = None
    HAS_TORCH = False
else:
    HAS_TORCH = True

If import torch succeeds but import torch.cuda fails (edge case: corrupted install), the except block sets torch = None, discarding the successfully-imported torch module. This is wasteful — torch could be used for CPU ops even without CUDA.

Better:

try:
    import torch
except ImportError:
    torch = None
    HAS_TORCH = False
else:
    try:
        import torch.cuda
    except ImportError:
        pass  # CUDA submodule missing, handled by check_requirements()
    HAS_TORCH = True

Finding 3 (🟡 Coverage gap): No test when torch IS available

The new test only covers the torch-missing path. Consider adding a second test that verifies the module behaves correctly when torch and torch.cuda are actually importable (e.g., with a mock that makes torch.cuda.is_available() return True). This would catch regressions in the happy path.


Finding 4 (🟢 Nit): from __future__ import annotations is unrelated

Added at the top of the file but not used by any change in this PR. This is PEP 604 style (str | None syntax). Fine if the project is standardizing, but it's unrelated noise in this diff.


What works well

  • check_requirements() pattern is clean — defers enforcement to call time, separating detection from action
  • run_gpu_fingerprint guards with check_requirements() at the entry point
  • CLI __main__ block wraps the whole thing in try/except RuntimeError for clean user-facing errors
  • Test uses importlib.util.spec_from_file_location correctly to avoid cached imports

Verdict

Request Changes — The test in test_gpu_fingerprint_import.py is broken: setting sys.modules["torch"] = None does not simulate a missing module. Fix the test approach to use delitem or builtins.__import__ mocking. The core gpu_fingerprint.py change is solid.

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great contribution to the RustChain ecosystem!

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR! The changes look good. 🎉

@Scottcjn

Scottcjn commented Jun 3, 2026

Copy link
Copy Markdown
Owner

✅ Merging — this is the clean split I asked for

Exactly the gpu_fingerprint.py import-safety fix from #6711 (lazy check_requirements() instead of import-time sys.exit), on its own with its import test — none of #6711's monitor-NameError or auth-on-public-GET changes. Files are only miners/gpu_fingerprint.py + tests/test_gpu_fingerprint_import.py. Thanks @yyswhsccc.

@Scottcjn Scottcjn merged commit 8239474 into Scottcjn:main Jun 3, 2026
11 checks passed

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Thanks for contributing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) size/M PR: 51-200 lines tests Test suite changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants