-
Notifications
You must be signed in to change notification settings - Fork 250
Loosen package dependencies #166
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
Changes from all commits
0c6a43e
5fc6e58
62518cc
d45ba87
a8d5b48
3a80983
14820d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,9 +34,9 @@ classifiers = [ | |
|
|
||
| dependencies = [ | ||
| "GPUtil", | ||
| "numpy<2.0.0", | ||
| "peft>=0.11.0,<0.12.0", | ||
| "pillow>=9.2.0,<11.0.0", | ||
| "numpy", | ||
| "peft>=0.11.0", | ||
| "pillow>=9.2.0", | ||
|
tonywu71 marked this conversation as resolved.
|
||
| "requests", | ||
| "torch>=2.2.0", | ||
| "transformers>=4.46.1,<4.47.0", | ||
|
|
@@ -49,7 +49,9 @@ train = [ | |
| "configue>=5.0.0", | ||
| "datasets>=2.19.1", | ||
| "mteb>=1.16.3,<1.17.0", | ||
| "typer>=0.12.3, <1.0.0", | ||
| "peft>=0.11.0,<0.12.0", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the idea of having double requirements here and above ? is there something I don't know about ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The point, which was raised by @galleon, is that our users might want to work with loosened dependencies, e.g. what if someone wants to use My proposition is:
While there might be some minor discrepancies in the results, I think it's inevitable and that my proposition achieves a good happy medium. But happy to hear your opinion and/or your replacement solutions on this!
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree that it would be nice to have less strict restrictions on inference only usage than training. I still think we need to upper bond the transformers version. People that want to use with newer versions can always install colpali-engine with --no-deps but otherwise at terms this will just lead to bugs I fear...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds like a good compromise, thanks! So following your propositions, I have added the upper bound for FYI I've tested the pyproject override behavior and it works just as planned: each dep takes the intersection of lower/upper bounds defined in the different groups. So installing |
||
| "pillow>=9.2.0,<11.0.0", | ||
| "typer>=0.15.1", | ||
| ] | ||
|
|
||
| interpretability = [ | ||
|
|
@@ -58,9 +60,13 @@ interpretability = [ | |
| "seaborn>=0.13.2,<1.0.0", | ||
| ] | ||
|
|
||
| dev = ["pytest>=8.0.0", "ruff>=0.4.0"] | ||
| dev = ["datasets>=2.19.1", "pytest>=8.0.0", "ruff>=0.4.0"] | ||
|
|
||
| all = ["colpali-engine[dev]", "colpali-engine[train]"] | ||
| all = [ | ||
| "colpali-engine[dev]", | ||
| "colpali-engine[interpretability]", | ||
| "colpali-engine[train]", | ||
| ] | ||
|
|
||
| [project.urls] | ||
| homepage = "https://github.com/illuin-tech/colpali" | ||
|
|
||
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.
bump to 12 but in this case, still bind 13 (or latest).
And test !!
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.
Is bumping to 12 really necessary?