Skip to content

Avoid optional requests import in TF converter - #28825

Merged
Justin Chu (justinchuby) merged 1 commit into
mainfrom
GopalakrishnanN/fix-transformer-conversion-test-ci
Jul 7, 2026
Merged

Justin Chu (justinchuby) merged 1 commit into
mainfrom
GopalakrishnanN/fix-transformer-conversion-test-ci

Conversation

@GopalakrishnanN

@GopalakrishnanN Gopalakrishnan Nallasamy (GopalakrishnanN) commented Jun 5, 2026 •

Copy link
Copy Markdown
Contributor

Description

Move the requests imports in convert_tf_models_to_pytorch.py into the two download paths that actually use them (download_compressed_file and the non-archive path in download_tf_checkpoint). This lets the archive-extraction tests import the module without requiring requests to be installed.

Motivation

The transformer conversion tests import the converter module at pytest collection time (from convert_tf_models_to_pytorch import safe_extract_archive) purely to exercise safe_extract_archive. However, requests is not a declared dependency of the transformers tests — onnxruntime/python/tools/transformers/requirements.txt lists onnx, numpy, transformers, torch, etc., but not requests (it is normally present only transitively via transformers).

When requests is absent, the module-level import requests raises ModuleNotFoundError at collection time, which fails the safe_extract_archive traversal/symlink tests even though those tests never touch the download paths that use requests. Deferring the import into the download functions decouples the archive-safety tests from this undeclared optional dependency.

Note: this is about dependency availability, not import speed — importing requests is cheap (its own module code is ~1 ms; the full transitive tree is a one-time ~0.25 s cold, mostly stdlib). Adding requests to the transformers test requirements would be an equivalent fix; the lazy import is just the smaller, more localized change.

Testing

  • Reproduced the original failure in a venv without requests: importing the module raises ModuleNotFoundError: No module named 'requests'. After moving the imports, safe_extract_archive imports and runs without requests.
  • Direct safe_extract_archive tar/zip traversal checks passed with .\.venv\Scripts\python.exe.
  • .\.venv\Scripts\python.exe -m pytest onnxruntime\test\python\transformers\test_convert_tf_models_to_pytorch.py -q was attempted locally but collection requires torch, which is not installed in this venv.

Copilot AI 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.

Pull request overview

This PR makes the TensorFlow-to-PyTorch converter module cheaper to import by deferring the optional requests dependency until the specific download code paths execute. This supports the transformer conversion test suite that imports the module during test collection to exercise safe_extract_archive without requiring requests.

Changes:

  • Removed the top-level requests import to avoid requiring the dependency at module import time.
  • Added local import requests # noqa: PLC0415 statements inside download_compressed_file and the non-archive download path in download_tf_checkpoint.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@tianleiwu

Copy link
Copy Markdown
Contributor

I do not see real issues in original code. Do we need this change?

@GopalakrishnanN

Copy link
Copy Markdown
Contributor Author

Copilot, Is this code change needed?

Copilot AI commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Copilot, Is this code change needed?

Strictly speaking, no — the original top-level import requests has no bug, and Tianlei Wu (@tianleiwu) is right that there's no real issue with that code.

The only thing this change buys is making the module importable when requests isn't installed. The test (test_convert_tf_models_to_pytorch.py) imports the module at collection time but only uses safe_extract_archive, and requests is not listed in the transformers test requirements. So the lazy import avoids a hard dependency on requests purely for archive-extraction tests. I confirmed the module imports cleanly without requests after this change, and fails to import without it before the change.

If the CI test environment already has requests available (it commonly is, as a transitive dependency), this change isn't necessary and can be reverted. If you'd prefer to drop it, let me know and I'll revert; otherwise it's a harmless, conventional deferral of an optional dependency.

@GopalakrishnanN

Copy link
Copy Markdown
Contributor Author

I do not see real issues in original code. Do we need this change?

There was a failure in the CI pipeline when I created a PR, the reason was one of the import libraries('requests') failed. When I analyzed the issue with AI, it suggested to move the import statements to the functions where it was getting used, there by avoiding this particular import until it was actually needed.

@justinchuby

Copy link
Copy Markdown
Contributor

Is importing requests expensive though?

@GopalakrishnanN

Copy link
Copy Markdown
Contributor Author

Justin Chu (@justinchuby) — you're right, it isn't. requests' own module code is ~1 ms to import, and even a cold import of its whole transitive tree (urllib3, ssl, http.client, email, certifi, idna) is only ~0.25 s on my Windows box — most of that is stdlib other imports pull in anyway, and it's a one-time cost. So import time isn't the real motivation, and the "lightweight import" wording in my original description oversold it (I've updated the description).

The actual reason is dependency availability, not speed. requests isn't a declared dependency of the transformers tests — onnxruntime/python/tools/transformers/requirements.txt lists onnx, numpy, transformers, torch, etc., but not requests; it's normally present only transitively via transformers. When it's absent, the module-level import requests raises ImportError at pytest collection time, which fails the safe_extract_archive traversal/symlink tests even though those tests never touch the download paths that use requests.

Reproduced locally in a venv without requests:

ModuleNotFoundError: No module named 'requests'

Moving the two import requests statements into download_compressed_file / download_tf_checkpoint lets the extraction tests import and run without the dependency.

So this decouples the archive-safety tests from an undeclared optional dependency — it isn't a perf optimization. If you'd prefer to keep the top-level import, the equivalent fix is to add requests to the transformers test requirements.txt. The lazy import is just the smaller, more localized change (flagged with # noqa: PLC0415) and follows the usual "defer the optional dependency" pattern. Happy to go whichever way you prefer.

@justinchuby
Justin Chu (justinchuby) merged commit 04cc747 into main Jul 7, 2026
91 of 92 checks passed
@justinchuby
Justin Chu (justinchuby) deleted the GopalakrishnanN/fix-transformer-conversion-test-ci branch July 7, 2026 02:35
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.

5 participants