-
Notifications
You must be signed in to change notification settings - Fork 48
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
deps: limit dependency ranges #54
Conversation
- automatic minor upgrades, must check for major Signed-off-by: Anh-Uong <[email protected]>
- splitup flash-attn and core requirements Signed-off-by: Anh-Uong <[email protected]>
relates/depends on #68 |
requirements.txt
Outdated
sentencepiece>=0.1.99,<2.0 | ||
tokenizers>=0.13.3,<1.0 | ||
tqdm>=4.66.2,<5.0 | ||
trl>=0.7.10,<1.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.
for libraries that are not yet 1.0
, I think it is not guaranteed that there won't be any API breaking change between subversions, so it is probably better to be more conservative about their versions in range.
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.
applies less for accelerate
though, since that library just never went 1.0
but is very popular, so unlikely to make such changes now without big splash.
Signed-off-by: Anh-Uong <[email protected]>
Signed-off-by: Anh-Uong <[email protected]>
Signed-off-by: Anh-Uong <[email protected]>
Signed-off-by: Anh-Uong <[email protected]>
@gkumbhat do you mind giving this another review? I agree with your point that
but I'm not sure what more conservative version to set as the upper limit, any suggestions? |
@tedhtchang if you can also give this another review pass, many thanks! |
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.
@anhuong Could you rebase.
For the important packages, I think we could set more conservative limit for those lower than 1.0.
In new PR we can figure out how we freeze packages versions in case we need to create older commit.
pyproject.toml
Outdated
"fire", | ||
"simpleeval", | ||
"numpy>=1.26.4,<2.0", | ||
"accelerate>=0.20.3,<1.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.
perhaps set more conservative 0.4 (current 0.31) since it's lower than 1.0
pyproject.toml
Outdated
"accelerate>=0.20.3,<1.0", | ||
"transformers>=4.34.1,<5.0,!=4.38.2", | ||
"torch>=2.2.0,<3.0", | ||
"sentencepiece>=0.1.99,<1.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.
perhaps set more conservative limit say 0.3 (current 0.2). This package updates very slow.
pyproject.toml
Outdated
"sentencepiece>=0.1.99,<1.0", | ||
"tokenizers>=0.13.3,<1.0", | ||
"tqdm>=4.66.2,<5.0", | ||
"trl>=0.7.10,<1.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.
perhaps 0.10.0 current is 0.9.4
pyproject.toml
Outdated
"tokenizers>=0.13.3,<1.0", | ||
"tqdm>=4.66.2,<5.0", | ||
"trl>=0.7.10,<1.0", | ||
"peft>=0.8.0,<1.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.
perhaps set limit to 0.13 current 0.11.1
Signed-off-by: Anh-Uong <[email protected]>
Signed-off-by: Anh-Uong <[email protected]>
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.
/LGTM
Signed-off-by: Anh-Uong <[email protected]>
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.
/LGTM
Limit dependency ranges to automatic minor upgrades, must manually check for major upgrades to see what breaking changes are included. Matches current versions that are being used