-
Notifications
You must be signed in to change notification settings - Fork 55
Unpin submodlib-py and remove linux only install constraint #603
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
Unpin submodlib-py and remove linux only install constraint #603
Conversation
Signed-off-by: eshwarprasadS <eshwarprasad.s01@gmail.com>
|
I'm wondering if at this point we remove the version constrain entirely? Or something like |
This sounds fine as well. Only reason I am wary of uncapped versioning for the dependency is due to its potential instability, specifically for this dependency. Not strictly opposed to this, just leaned towards the safer side, since we have had previous issues with this library and its stability. Open to removing the cap entirely for now, given this, WDYT? |
|
I think removing the cap entirely is reasonable for now, and we can address if some incompatible releases come out while we're still supporting this code. |
requirements.txt
Outdated
| sentencepiece>=0.2.0 | ||
| # Note: this dependency has to be built from source | ||
| submodlib-py==0.0.1; sys_platform == 'linux' | ||
| submodlib-py==0.0.2 |
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.
Please do not pin to an exact version like this. If we had to deliver a fix in submodlib-py downstream we would not be able to ship the incompatible versions of the library. There are some guidelines for managing versions documented in https://github.com/instructlab/dev-docs/blob/main/docs/dependency-management.md.
I think in this case a version range like <0.1.0 or even <1.0.0 would make the most sense.
If you want to pin to a specific version for your CI jobs, use a constraints file separate from the requirements file.
|
@eshwarprasadS I see the small E2E job is failing. In our core repo, we had to temporarily disable this job: instructlab/instructlab#3290 See linked issue for more information about why we temporarily disabled it: instructlab/instructlab#3289 So we can ignore the small E2E job failure here once @dhellmann's review comments are addressed. |
Signed-off-by: eshwarprasadS <eshwarprasad.s01@gmail.com>
would we want to get another PR in for the e2e job failure, or can we ignore and merge for now, if the comments here are resolved? |
|
I suggest we merge this PR (with the small E2E failure) if it looks good to everyone, and comment out the small E2E job in another PR. This way, we don’t block this PR’s progress. @bbrowning can you review this PR once more? |
|
Wonderful, thanks everyone, for all the help in getting this in. Will put up another PR for disabling the small E2E job (for the time being). |
|
@Mergifyio backport release-v0.8 |
✅ Backports have been createdDetails
|
Unpin submodlib-py and remove linux only install constraint (backport #603)
Since there is a new
0.0.2release tag fromsubmodlib-py, remove requirement versioning pin and remove linux-only requirement (as 0.0.2 supports MacOS builds)