Skip to content
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

pyproject.toml #1074

Merged
merged 8 commits into from
Mar 11, 2024
Merged

pyproject.toml #1074

merged 8 commits into from
Mar 11, 2024

Conversation

rasbt
Copy link
Collaborator

@rasbt rasbt commented Mar 8, 2024

Fixes #1071

I was banging my head against it for more than an hour and came to the conclusion that pyproject does not support dynamic optional dependencies yet. For now, they have to be static. It's maybe kind of overkill, but I added a pyproject_toml_generate.py script that updated the .toml optional dependencies from the requirements-all.txt.

The question is, should we not have this pyproject_toml_generate.py? But then we will have to update 2 places manually.

Otherwise, instead of recommending pip install -r requirements-all.txt we just recommend pip install ".[all]"

Usage

So, the general usage is this

Users

# basic installation
pip install -e .

# install with all dependencies
pip install -e ".[all]"

Developers (if you want to build the PyPI package)

# install twine and build if needed
pip install twine build

# update toml in case requirements-all.txt changed
python pyproject_toml_generate.py

# build the distribution
python -m build  

(This will create both the source code in tar.gz and the wheels)


# Test PyPI upload:

pip install -i https://testpypi.python.org/pypi litgpt

# Then the real PyPI upload:

python -m twine upload  dist/*

Should we document this somewhere?

@rasbt rasbt changed the base branch from main to wip March 8, 2024 21:29
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having this file is very confusing and complex. If we want a single place where dependencies are updated then we should get rid of the requirements files.

https://stackoverflow.com/a/73600610 says that we can use files for optional dependencies but then we would need to add a requirements-test.txt file too because test dependencies are currently only defined in pyproject.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That file also says

Note that the referenced file will use a requirements.txt-like syntax; each line must conform to PEP 508, so flags like -r, -c, and -e are not supported inside this requirements.txt.

So would need to drop the -r we have, making it so users always need to do

pip install -r requirements.txt -r requirements-all.txt.

In my opinion, this is impractical and would stick to a single file that contains all the package metadata

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm yeah, 3 requirements files + .toml is probably too much. Should I combine everything into the .toml directly? I think if we add a clear installation instruction

# basic installation
pip install -e .

# install with all dependencies
pip install -e ".[all]"

# install test dependencies
pip install -e ".[test]"

then I guess it should be fine not having requirements.txt files. I think .toml files are very readable. And ours is pretty small too.

@rasbt
Copy link
Collaborator Author

rasbt commented Mar 11, 2024

Should be updated now so that everything is self-contained in the `pyproject.toml'

requirements-all.txt Show resolved Hide resolved
@carmocca carmocca merged commit e0886f2 into wip Mar 11, 2024
8 checks passed
@carmocca carmocca deleted the toml branch March 11, 2024 17:40
awaelchli pushed a commit that referenced this pull request Mar 15, 2024
awaelchli pushed a commit that referenced this pull request Mar 15, 2024
awaelchli pushed a commit that referenced this pull request Mar 15, 2024
rasbt added a commit that referenced this pull request Mar 18, 2024
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.

2 participants