-
Notifications
You must be signed in to change notification settings - Fork 249
[PyProject] Spin up an initial pyproject.toml allowing for local pip install #1527
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
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchchat/1527
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit fbe31ff with merge base bf05236 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
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.
Awesome work! Left some nit and a question.
pyproject.toml
Outdated
"tomli>=1.1.0; python_version < '3.11'", | ||
"openai", | ||
"wheel", | ||
"cmake>=3.24,<4.0.0", |
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.
nit: can keep the comments as we have in requirements.txt
.
# | ||
# First install requirements in install/requirements.txt. Older torch may be | ||
# installed from the dependency of other models. It will be overridden by | ||
# newer version of torch nightly installed later in this script. | ||
# |
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.
nit: keep this?
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.
Intentionally dropped the extra index since it's not actually a case that's being used (i.e. difference in version requirements based on pt version)
fi | ||
fi | ||
echo "Using python executable: $PYTHON_EXECUTABLE" | ||
|
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.
qq: why do we not need to copy over
torchchat/install/install_requirements.sh
Lines 23 to 33 in 12d1550
PYTHON_SYS_VERSION="$($PYTHON_EXECUTABLE -c "import sys; print(f'{sys.version_info.major}.{sys.version_info.minor}')")" | |
# Check python version. Expect at least 3.10.x | |
if ! $PYTHON_EXECUTABLE -c " | |
import sys | |
if sys.version_info < (3, 10): | |
sys.exit(1) | |
"; | |
then | |
echo "Python version must be at least 3.10.x. Detected version: $PYTHON_SYS_VERSION" | |
exit 1 | |
fi |
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 version check is actually going to be removed once we move to pyproject (enforces python version), so if anything it will be deleted
Co-authored-by: Zhenyan Zhang (Meta) <[email protected]>
LGTM, feel free to merge. |
This is the first step towards pip-installing of torchchat. Specifically this PR:
requirements.txt
torch
installation instructions frominstall_requirements.sh
intoinstall_torch.sh
torch
installation will be done separately from pip installNote that install instructions have not changed pending refactoring the following from
install_requirements.sh
Testing: