-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Fix tutorial 18584 #18696
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
Fix tutorial 18584 #18696
Conversation
- Add prerequisite note about building TVM with CUDA support - Add error handling for 'tvm_ffi' import failure with helpful message - Direct users to build instructions when TVM is not built - Fixes issue apache#18584 where users get confusing import errors Test Plan: - File compiles without syntax errors - Error message guides users to correct solution
Summary of ChangesHello @AshwiniBokka, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on enhancing the user experience and reliability of a specific tutorial by addressing a common dependency issue. It ensures that users encountering problems related to CUDA support receive clear guidance. Additionally, it includes minor but important updates to license links, promoting secure access to Apache License documentation. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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 primarily updates HTTP links to HTTPS across the repository, which is a good security improvement. It also enhances the customize_opt.py tutorial by adding a check for CUDA support, improving the user experience. My review includes a few suggestions for this tutorial script to align with best practices, such as removing an unnecessary Byte Order Mark, directing error messages to stderr, and cleaning up trailing newlines.
| @@ -1,4 +1,4 @@ | |||
| # Licensed to the Apache Software Foundation (ASF) under one | |||
| # Licensed to the Apache Software Foundation (ASF) under one | |||
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.
This line contains a Unicode Byte Order Mark (BOM) \ufeff, which is unnecessary for UTF-8 encoded files and can cause issues with some tools. It should be removed.
| # Licensed to the Apache Software Foundation (ASF) under one | |
| # Licensed to the Apache Software Foundation (ASF) under one |
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.
pls take a look
| print("Error: TVM needs to be built with CUDA support.") | ||
| print("Please build TVM from source with CUDA enabled.") | ||
| print("See: https://tvm.apache.org/docs/install/from_source.html") | ||
| import sys |
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.
Error messages should be printed to standard error (stderr) instead of standard output (stdout). This is a standard practice for separating diagnostic output from normal program output. Also, it's better to move the import sys statement to the top of the except block.
| print("Error: TVM needs to be built with CUDA support.") | |
| print("Please build TVM from source with CUDA enabled.") | |
| print("See: https://tvm.apache.org/docs/install/from_source.html") | |
| import sys | |
| import sys | |
| print("Error: TVM needs to be built with CUDA support.", file=sys.stderr) | |
| print("Please build TVM from source with CUDA enabled.", file=sys.stderr) | |
| print("See: https://tvm.apache.org/docs/install/from_source.html", file=sys.stderr) |
| # iterate the optimization and improve the performance of the model. | ||
| # | ||
|
|
||
|
|
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.
- Properly indent except ImportError block - Remove duplicate import sys statement - Ensure if/else block has correct indentation - All error messages use sys.stderr - File now compiles without syntax errors Addresses all code review feedback
| @@ -1,4 +1,4 @@ | |||
| # Licensed to the Apache Software Foundation (ASF) under one | |||
| # Licensed to the Apache Software Foundation (ASF) under one | |||
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.
pls take a look
| from tvm.relax.frontend import nn | ||
|
|
||
| # Note: This tutorial requires TVM to be built with CUDA support. | ||
| # If you encounter 'No module named ''tvm_ffi''' error, |
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.
tvm_ffi can be installed with cd 3rdparty/tvm-ffi; pip install .
| Apache License | ||
| Version 2.0, January 2004 | ||
| http://www.apache.org/licenses/ | ||
| https://www.apache.org/licenses/ |
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 LICENSE update isn’t related to this PR. I’d suggest we avoid changing it here and handle it in a separate PR if needed.
|
|
||
| try: | ||
| import tvm.relax.backend.cuda.cublas as _cublas | ||
| except ImportError as e: |
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.
probably we don't have to handle the tvm_ffi specifically here
No description provided.