-
-
Notifications
You must be signed in to change notification settings - Fork 11.7k
Revert #29787 and #29690 #29815
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert #29787 and #29690 #29815
Conversation
This reverts commit 36db0a3.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Documentation preview: https://vllm--29815.org.readthedocs.build/en/29815/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request reverts #29787 and #29690 to fix a broken Python installation test on the main branch. The changes effectively roll back a complex wheel-building and publishing system that supported multiple variants, and restore a simpler mechanism. This involves removing the advanced index generation script, simplifying the upload script and the logic in setup.py for handling precompiled wheels. The documentation has also been updated to reflect these changes, including removing instructions for pre-built CPU wheels and adjusting CUDA version references. The default CUDA version is also reverted to 12.8. The revert appears to be comprehensive and consistent across the codebase. I've found one issue in the new logic in setup.py for fetching precompiled wheels that should be addressed.
| from urllib.request import urlopen | ||
|
|
||
| try: | ||
| wheels, repo_url = _fetch_metadata_for_variant(commit, variant) | ||
| except Exception: | ||
| logger.warning( | ||
| "Failed to fetch precompiled wheel metadata for variant %s", | ||
| variant, | ||
| exc_info=True, | ||
| ) | ||
| try_default = True # try outside handler to keep the stacktrace simple | ||
| if try_default: | ||
| logger.info("Trying the default variant") | ||
| wheels, repo_url = _fetch_metadata_for_variant(commit, None) | ||
| # if this also fails, then we have nothing more to try / cache | ||
| assert wheels is not None and repo_url is not None, ( | ||
| "Failed to fetch precompiled wheel metadata" | ||
| ) | ||
| # The metadata.json has the following format: | ||
| # see .buildkite/scripts/generate-nightly-index.py for details | ||
| """[{ | ||
| "package_name": "vllm", | ||
| "version": "0.11.2.dev278+gdbc3d9991", | ||
| "build_tag": null, | ||
| "python_tag": "cp38", | ||
| "abi_tag": "abi3", | ||
| "platform_tag": "manylinux1_x86_64", | ||
| "variant": null, | ||
| "filename": "vllm-0.11.2.dev278+gdbc3d9991-cp38-abi3-manylinux1_x86_64.whl", | ||
| "path": "../vllm-0.11.2.dev278%2Bgdbc3d9991-cp38-abi3-manylinux1_x86_64.whl" | ||
| }, | ||
| ...]""" | ||
| for wheel in wheels: | ||
| # TODO: maybe check more compatibility later? (python_tag, abi_tag, etc) | ||
| if wheel.get("package_name") == "vllm" and arch in wheel.get( | ||
| "platform_tag", "" | ||
| ): | ||
| logger.info("Found precompiled wheel metadata: %s", wheel) | ||
| if "path" not in wheel: | ||
| raise ValueError(f"Wheel metadata missing path: {wheel}") | ||
| wheel_url = repo_url + wheel["path"] | ||
| download_filename = wheel.get("filename") | ||
| logger.info("Using precompiled wheel URL: %s", wheel_url) | ||
| break | ||
| else: | ||
| raise ValueError( | ||
| f"No precompiled vllm wheel found for architecture {arch} " | ||
| f"from repo {repo_url}. All available wheels: {wheels}" | ||
| ) | ||
| patch = precompiled_wheel_utils.extract_precompiled_and_patch_package( | ||
| wheel_url, download_filename | ||
| ) | ||
| with urlopen(wheel_url) as resp: | ||
| if resp.status != 200: | ||
| wheel_url = nightly_wheel_url | ||
| except Exception as e: | ||
| print(f"[warn] Falling back to nightly wheel: {e}") | ||
| wheel_url = nightly_wheel_url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for falling back to the nightly wheel URL is flawed. The if resp.status != 200: check inside the try block is unreachable, because urllib.request.urlopen raises an HTTPError (a subclass of URLError) for non-2xx HTTP status codes. This means the if condition will never be met for a failed request. Additionally, catching the generic Exception is too broad and can mask other issues. A more robust approach is to rely solely on exception handling for URLError to manage the fallback.
| from urllib.request import urlopen | |
| try: | |
| wheels, repo_url = _fetch_metadata_for_variant(commit, variant) | |
| except Exception: | |
| logger.warning( | |
| "Failed to fetch precompiled wheel metadata for variant %s", | |
| variant, | |
| exc_info=True, | |
| ) | |
| try_default = True # try outside handler to keep the stacktrace simple | |
| if try_default: | |
| logger.info("Trying the default variant") | |
| wheels, repo_url = _fetch_metadata_for_variant(commit, None) | |
| # if this also fails, then we have nothing more to try / cache | |
| assert wheels is not None and repo_url is not None, ( | |
| "Failed to fetch precompiled wheel metadata" | |
| ) | |
| # The metadata.json has the following format: | |
| # see .buildkite/scripts/generate-nightly-index.py for details | |
| """[{ | |
| "package_name": "vllm", | |
| "version": "0.11.2.dev278+gdbc3d9991", | |
| "build_tag": null, | |
| "python_tag": "cp38", | |
| "abi_tag": "abi3", | |
| "platform_tag": "manylinux1_x86_64", | |
| "variant": null, | |
| "filename": "vllm-0.11.2.dev278+gdbc3d9991-cp38-abi3-manylinux1_x86_64.whl", | |
| "path": "../vllm-0.11.2.dev278%2Bgdbc3d9991-cp38-abi3-manylinux1_x86_64.whl" | |
| }, | |
| ...]""" | |
| for wheel in wheels: | |
| # TODO: maybe check more compatibility later? (python_tag, abi_tag, etc) | |
| if wheel.get("package_name") == "vllm" and arch in wheel.get( | |
| "platform_tag", "" | |
| ): | |
| logger.info("Found precompiled wheel metadata: %s", wheel) | |
| if "path" not in wheel: | |
| raise ValueError(f"Wheel metadata missing path: {wheel}") | |
| wheel_url = repo_url + wheel["path"] | |
| download_filename = wheel.get("filename") | |
| logger.info("Using precompiled wheel URL: %s", wheel_url) | |
| break | |
| else: | |
| raise ValueError( | |
| f"No precompiled vllm wheel found for architecture {arch} " | |
| f"from repo {repo_url}. All available wheels: {wheels}" | |
| ) | |
| patch = precompiled_wheel_utils.extract_precompiled_and_patch_package( | |
| wheel_url, download_filename | |
| ) | |
| with urlopen(wheel_url) as resp: | |
| if resp.status != 200: | |
| wheel_url = nightly_wheel_url | |
| except Exception as e: | |
| print(f"[warn] Falling back to nightly wheel: {e}") | |
| wheel_url = nightly_wheel_url | |
| from urllib.request import urlopen | |
| from urllib.error import URLError | |
| try: | |
| # Attempt to open the URL. urlopen will raise URLError for HTTP errors. | |
| with urlopen(wheel_url): | |
| pass | |
| except URLError as e: | |
| print(f"[warn] Could not find wheel at {wheel_url}, falling back to nightly wheel: {e}") | |
| wheel_url = nightly_wheel_url |
Reverting since Python Installation test is broken on main