Fix gdown fails #8576
Fix gdown fails #8576
Conversation
Signed-off-by: Eric Kerfoot <[email protected]>
Signed-off-by: Eric Kerfoot <[email protected]>
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ericspod
requested review from
virginiafdez,
KumoLiu and
Nic-Ma
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
tests/test_utils.py (5)
68-73: Broaden exception set to cover requests connection/timeouts.Include
requests.ConnectionErrorandrequests.Timeoutwhen available.http_error, has_req = optional_import("requests", name="HTTPError") file_url_error, has_gdown = optional_import("gdown.exceptions", name="FileURLRetrievalError") +requests_conn_error, _ = optional_import("requests", name="ConnectionError") +requests_timeout, _ = optional_import("requests", name="Timeout") DOWNLOAD_EXCEPTS = (ContentTooShortError, HTTPError, ConnectionError) if has_req: - DOWNLOAD_EXCEPTS += (http_error,) + DOWNLOAD_EXCEPTS += tuple(x for x in (http_error, requests_conn_error, requests_timeout) if x) if has_gdown: DOWNLOAD_EXCEPTS += (file_url_error,)
74-84: Heuristic list: add common rate-limit/quota signals and use case-insensitive matching.Covers the linked GDrive “too many users” and generic 429/RL cases.
-DOWNLOAD_FAIL_MSGS = ( - "unexpected EOF", # incomplete download - "network issue", - "gdown dependency", # gdown not installed - "md5 check", - "limit", # HTTP Error 503: Egress is over the account limit - "authenticate", - "timed out", # urlopen error [Errno 110] Connection timed out - "HTTPError", # HTTPError: 429 Client Error: Too Many Requests for huggingface hub -) +DOWNLOAD_FAIL_MSGS = ( + "unexpected eof", + "network issue", + "gdown dependency", + "md5 check", + "limit", + "authenticate", + "timed out", + "httperror", + "429", + "too many requests", + "too many users", + "quota", + "quota exceeded", +)And normalize error strings below (see next comment).
164-167: Docstring: tighten and align to Google style (brief).def skip_if_downloading_fails(): - """ - Skips a test if downloading something raises an exception recognised to indicate a download has failed. - """ + """Skip a test when a recognized download/network failure occurs. + + Yields: + None: Acts as a context manager around code that may download. + + Notes: + Skips on exceptions in DOWNLOAD_EXCEPTS or when error text matches DOWNLOAD_FAIL_MSGS. + """
170-171: Ruff TRY003: silence or shorten message.Keep the context but appease the linter.
- except DOWNLOAD_EXCEPTS as e: - raise unittest.SkipTest(f"Error while downloading: {e}") from e + except DOWNLOAD_EXCEPTS as e: + raise unittest.SkipTest(f"Download error: {e}") from e # noqa: TRY003Also consider adding the same noqa on the SSL branch below.
177-179: Case-insensitive matching + TRY003.Lowercase once, compare lowercased hints, and silence TRY003.
- except (RuntimeError, OSError) as rt_e: - err_str = str(rt_e) - if any(k in err_str for k in DOWNLOAD_FAIL_MSGS): - raise unittest.SkipTest(f"Error while downloading: {rt_e}") from rt_e # incomplete download + except (RuntimeError, OSError) as rt_e: + err_norm = str(rt_e).lower() + if any(k in err_norm for k in DOWNLOAD_FAIL_MSGS): + raise unittest.SkipTest(f"Download error: {rt_e}") from rt_e # noqa: TRY003
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/losses/perceptual.py(2 hunks)tests/test_utils.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/losses/perceptual.pytests/test_utils.py
🪛 Ruff (0.12.2)
tests/test_utils.py
171-171: Avoid specifying long messages outside the exception class
(TRY003)
174-174: Avoid specifying long messages outside the exception class
(TRY003)
178-178: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: packaging
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (macOS-latest)
🔇 Additional comments (1)
tests/test_utils.py (1)
60-60: Good: catch gdown’s FileURLRetrievalError.
Signed-off-by: Eric Kerfoot <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/test_utils.py (3)
74-83: Harden failure heuristics; use case-insensitive matching and add quota/429 tokens.Prevents misses (“Too Many Users/Requests”, 429) and removes case sensitivity.
Apply:
-DOWNLOAD_FAIL_MSGS = ( - "unexpected EOF", # incomplete download - "network issue", - "gdown dependency", # gdown not installed - "md5 check", - "limit", # HTTP Error 503: Egress is over the account limit - "authenticate", - "timed out", # urlopen error [Errno 110] Connection timed out - "HTTPError", # HTTPError: 429 Client Error: Too Many Requests for huggingface hub -) +DOWNLOAD_FAIL_MSGS = ( + "unexpected eof", # incomplete download + "network issue", + "gdown dependency", # gdown not installed + "md5 check", + "quota", # GDrive quota exceeded + "too many users", + "too many requests", + "429", + "503", + "authenticate", + "timed out", # urlopen error [Errno 110] Connection timed out + "httperror", # generic HTTPError string +)And update the check (see lines 176–179):
-err_str = str(rt_e) -if any(k in err_str for k in DOWNLOAD_FAIL_MSGS): +err_str = str(rt_e) +err_lower = err_str.lower() +if any(k in err_lower for k in DOWNLOAD_FAIL_MSGS): raise unittest.SkipTest(f"Error while downloading: {rt_e}") from rt_e # incomplete download
164-167: Docstring: clarify behavior (Google style).Add Yields/Raises for quick scan.
Apply:
-""" -Skips a test if downloading something raises an exception recognised to indicate a download has failed. -""" +""" +Context manager to skip a test when a recognized download failure occurs. + +Yields: + None + +Raises: + unittest.SkipTest: If a known download-related failure is detected. +"""
170-179: Shorten SkipTest messages (Ruff TRY003) while preserving cause via chaining.Keeps logs tight; details remain in the chained exception.
Apply:
-except DOWNLOAD_EXCEPTS as e: - raise unittest.SkipTest(f"Error while downloading: {e}") from e +except DOWNLOAD_EXCEPTS as e: + raise unittest.SkipTest("Download failed; skipping.") from e @@ - if "decryption failed" in str(ssl_e): - raise unittest.SkipTest(f"SSL error while downloading: {ssl_e}") from ssl_e + if "decryption failed" in str(ssl_e): + raise unittest.SkipTest("SSL error during download; skipping.") from ssl_e @@ -err_str = str(rt_e) -if any(k in err_str for k in DOWNLOAD_FAIL_MSGS): - raise unittest.SkipTest(f"Error while downloading: {rt_e}") from rt_e # incomplete download +err_str = str(rt_e) +err_lower = err_str.lower() +if any(k in err_lower for k in DOWNLOAD_FAIL_MSGS): + raise unittest.SkipTest("Download failed; skipping.") from rt_e
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
tests/test_utils.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
tests/test_utils.py
🪛 Ruff (0.12.2)
tests/test_utils.py
171-171: Avoid specifying long messages outside the exception class
(TRY003)
174-174: Avoid specifying long messages outside the exception class
(TRY003)
178-178: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: build-docs
- GitHub Check: packaging
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (windows-latest)
🔇 Additional comments (1)
tests/test_utils.py (1)
59-60: Good: explicitly handling gdown’s FileURLRetrievalError.This directly addresses the flaky GDrive quota errors.
Signed-off-by: Eric Kerfoot <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/test_utils.py (1)
58-60: Broaden network exception coverage (requests + urllib).Missing requests.exceptions.ConnectionError/Timeout and urllib.error.URLError means common failures won’t be skipped. Also import HTTPError from requests.exceptions for clarity.
-from urllib.error import ContentTooShortError, HTTPError +from urllib.error import ContentTooShortError, HTTPError, URLError @@ -http_error, has_req = optional_import("requests", name="HTTPError") +http_error, has_req = optional_import("requests.exceptions", name="HTTPError") +req_conn_error, has_req_conn = optional_import("requests.exceptions", name="ConnectionError") +req_timeout, has_req_timeout = optional_import("requests.exceptions", name="Timeout") @@ -DOWNLOAD_EXCEPTS: tuple[type, ...] = (ContentTooShortError, HTTPError, ConnectionError) +DOWNLOAD_EXCEPTS: tuple[type, ...] = (ContentTooShortError, HTTPError, URLError, ConnectionError) if has_req: DOWNLOAD_EXCEPTS += (http_error,) +if has_req_conn: + DOWNLOAD_EXCEPTS += (req_conn_error,) +if has_req_timeout: + DOWNLOAD_EXCEPTS += (req_timeout,) if has_gdown: DOWNLOAD_EXCEPTS += (file_url_error,)Also applies to: 68-73
🧹 Nitpick comments (5)
tests/test_utils.py (5)
74-83: Tighten/extend failure substrings; avoid over-broad “limit”.Add specific quota/429 markers to reduce false positives while catching the reported Drive quota error.
DOWNLOAD_FAIL_MSGS = ( "unexpected EOF", # incomplete download "network issue", "gdown dependency", # gdown not installed "md5 check", - "limit", # HTTP Error 503: Egress is over the account limit + "limit", # keep generic for legacy messages + "quota", # hosting quota exceeded + "Too Many Requests", # 429 + "Too many users have viewed or downloaded this file", # Google Drive quota + "egress is over the account limit", # 503 egress limit "authenticate", "timed out", # urlopen error [Errno 110] Connection timed out "HTTPError", # HTTPError: 429 Client Error: Too Many Requests for huggingface hub )
164-167: Docstring: clarify behavior/contract.State that it yields and raises unittest.SkipTest on recognized download failures.
def skip_if_downloading_fails(): - """ - Skips a test if downloading something raises an exception recognised to indicate a download has failed. - """ + """ + Context manager to skip a test when a recognized download failure occurs. + + Yields: + None + + Raises: + unittest.SkipTest: if a network/download-related error is detected. + """
170-171: Satisfy Ruff TRY003; use original message.Drop the custom prefix; keep the original exception message.
- except DOWNLOAD_EXCEPTS as e: - raise unittest.SkipTest(f"Error while downloading: {e}") from e + except DOWNLOAD_EXCEPTS as e: + raise unittest.SkipTest(str(e)) from e
172-175: Same TRY003 tweak for SSLError.Keep the message but avoid the prefixed string.
- except ssl.SSLError as ssl_e: - if "decryption failed" in str(ssl_e): - raise unittest.SkipTest(f"SSL error while downloading: {ssl_e}") from ssl_e + except ssl.SSLError as ssl_e: + if "decryption failed" in str(ssl_e): + raise unittest.SkipTest(str(ssl_e)) from ssl_e
177-178: Case-insensitive match; same TRY003 tweak.Avoid missing signals due to casing; keep original message.
- if any(k in err_str for k in DOWNLOAD_FAIL_MSGS): - raise unittest.SkipTest(f"Error while downloading: {rt_e}") from rt_e # incomplete download + if any(k.lower() in err_str.lower() for k in DOWNLOAD_FAIL_MSGS): + raise unittest.SkipTest(str(rt_e)) from rt_e # incomplete download
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
tests/test_utils.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
tests/test_utils.py
🪛 Ruff (0.12.2)
tests/test_utils.py
171-171: Avoid specifying long messages outside the exception class
(TRY003)
174-174: Avoid specifying long messages outside the exception class
(TRY003)
178-178: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: packaging
- GitHub Check: build-docs
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-os (macOS-latest)
|
/build |
Fixes #8549.
Description
This modifies
skip_if_downloading_failsto skip tests ifgdowndownloading fails. This should be used to temporarily account for failing tests when downloads start failing for whatever reason. A more reliable solution for hosting this data should be found so that tests can reliably download correctly, otherwise tests can be skipped and silently allow regression.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.