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

added poetry dependency management #207

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

leo-gan
Copy link

@leo-gan leo-gan commented May 26, 2023

I've added poetry.
poetry flawlessly installed all dependencies without any additional manual installation steps (with the poetry install command), including torch nvidia-cuda..., etc.
poetry checks all interdependencies between packages and locks all package versions in poetry.lock file. It is a much safer way to deal with dependencies.
If you think it makes sense to switch to poetry, I'll add the necessary descriptions in the Readme file.
Thanks!

@okhat
Copy link
Collaborator

okhat commented May 26, 2023

Wow, what a great timing! Looking more into this...

@okhat
Copy link
Collaborator

okhat commented May 26, 2023

Did you intend to push the lock file?

Also, what about gcc? That's sometimes needed if the gcc is old

@leo-gan
Copy link
Author

leo-gan commented May 26, 2023

Did you intend to push the lock file?

Also, what about gcc? That's sometimes needed if the gcc is old

About poetry.lock: yes. Pushing it in project files ensures equal package versioning for everybody. It recommended
UPDATE: see here.
About gcc: not sure about any binaries. In my experience, they can be better managed in Dockerfiles.
UPDATE regarding this

numpy provides several wheel files for different os, cpu architecture and python versions. wheel packages are precompiled, so the target system doesn't have to compile the package.

poetry is able to choose the right wheel for you, depending on your system.

@okhat
Copy link
Collaborator

okhat commented May 26, 2023

But doesn't the lock file include highly OS-specific things? Maybe I'm reading too much into all these linux links.

For the gcc thing, I guess we can ask people to conda install recent gcc if their version is too old. Manually.

Does that match what you have in mind?

Also, will this work with pip somehow? We'd like to add pip installation and want to make sure it runs in conda.

(My apologies for the many small questions. I can look into any of them that you don't have time to answer/explore.)

Happy to merge once we resolve this set of questions.

@leo-gan
Copy link
Author

leo-gan commented May 26, 2023

But doesn't the lock file include highly OS-specific things? Maybe I'm reading too much into all these linux links.

Actually, lock file includes wheels for all major OSs simultaneously. See, an example:

    {file = 
 "aiohttp-3.8.4-cp3
 10-cp310-musllinux_1_1_x86_64.whl", hash = "sha256:c6cc15d58053c76eacac5fa9152d7d84b8d67b3fde92709195cb984cfb3475ea"},
     {file = "aiohttp-3.8.4-cp310-cp310-win32.whl", hash = "sha256:59f029a5f6e2d679296db7bee982bb3d20c088e52a2977e3175faf31d6fb75d1"},
     {file = "aiohttp-3.8.4-cp310-cp310-win_amd64.whl", hash = "sha256:fe7ba4a51f33ab275515f66b0a236bcde4fb5561498fe8f898d4e549b2e4509f"},
     {file = "aiohttp-3.8.4-cp311-cp311-macosx_10_9_universal2.whl", hash = "sha256:3d8ef1a630519a2
6d6760bc695842579cb09e373c5f227a21b67dc3eb16cfea4"},

For the gcc thing, I guess we can ask people to conda install recent gcc if their version is too old. Manually.

I don't know exactly how, but poetry install didn't require any binaries in my installation. I assume, it just has found the correct ready-to-go weels.

Does that match what you have in mind?

Also, will this work with pip somehow? We'd like to add pip installation and want to make sure it runs in conda.

Yes, sure. We can combine poetry and pip as we wish. Just keep in mind, poetry can build a virtual environment out-of-box. See this.

(My apologies for the many small questions. I can look into any of them that you don't have time to answer/explore.)

Happy to merge once we resolve this set of questions.

@leo-gan
Copy link
Author

leo-gan commented May 26, 2023

One more. The poetry can also work as an addition, without any changes to existing infrastructure. This PR takes exactly this approach.

@leo-gan
Copy link
Author

leo-gan commented Sep 18, 2023

@okhat Could you, please, review it? Thanks

@Alexandre-SCHOEPP
Copy link

Alexandre-SCHOEPP commented Dec 18, 2023

But doesn't the lock file include highly OS-specific things? Maybe I'm reading too much into all these linux links.

The lock file is hardware/os independent. It's generally a bad idea to push it on libraries, however, since it constrains the dependencies to a single version. That is generally too restrictive outside of applications running in their dedicated environments (ref)

Also, will this work with pip somehow? We'd like to add pip installation and want to make sure it runs in conda.

poetry publish allows you to upload to pypi directly from poetry.

Poetry will generally work without a setup.py, as it is designed to use the newly (newer-ly, rather) pyproject.toml specification file.

@okhat
Copy link
Collaborator

okhat commented Dec 30, 2023

Ok @Alexandre-SCHOEPP so basically I can't merge this with the lock file, right? cc: @leo-gan what's a good way to deal with this?

@leo-gan
Copy link
Author

leo-gan commented Jan 23, 2024

Ok @Alexandre-SCHOEPP so basically I can't merge this with the lock file, right? cc: @leo-gan what's a good way to deal with this?

If we want reproducible tests (hence reproducible development) we need to merge the lock file into repo.
Many package repos include the lock file in the repo. Examples: LangChain, poetry itself.
Lock file in repo helps with reproducible development.
Lock file is not used when the package is installed but it doesn't hurt the installation. The only con is, it uses some space in the package size. Sometime, it is important, but in in this repo.
@okhat Merge the lock file. It is OK for the ColBERT.

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.

3 participants