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

add requirements management policy #122

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

dhellmann
Copy link
Contributor

@dhellmann dhellmann commented Jul 24, 2024

This document describes the policies for adding and updating build and runtime dependencies of all InstructLab components.

@dhellmann dhellmann force-pushed the requirements-management branch from f3993dc to 3961e0d Compare July 24, 2024 14:17
@dhellmann
Copy link
Contributor Author

@tiran
Copy link

tiran commented Jul 25, 2024

Several months ago, I experiemented with https://pypi.org/project/pip-compile-multi/ to generate a set of constraint files for our entire dependency chain. Yesterday I revived my old PR and tried again. pip-compile-multi breaks because packages like flash-attn have broken packaging metadata. Doug filed a ticket a while ago, but upstream hasn't agreed to accept the fix.

Related InstructLab ticket and PR to constraint versions:

@tiran
Copy link

tiran commented Jul 25, 2024

The topic of version bounds came up in internal channels. The blog post https://iscinumpy.dev/post/bound-version-constraints/ explains very well how to deal with version constraints in Python. It links to several other blog posts about semver and version handling in Python ecosystem, too.

Copy link
Member

@aakankshaduggal aakankshaduggal left a comment

Choose a reason for hiding this comment

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

Thanks @dhellmann!

Copy link
Member

@nathan-weinberg nathan-weinberg left a comment

Choose a reason for hiding this comment

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

few nits but otherwise LGTM

docs/dependency-management.md Outdated Show resolved Hide resolved
docs/dependency-management.md Outdated Show resolved Hide resolved
docs/dependency-management.md Outdated Show resolved Hide resolved
Copy link
Member

@n1hility n1hility left a comment

Choose a reason for hiding this comment

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

sounds perfect to me

Copy link
Contributor

@leseb leseb left a comment

Choose a reason for hiding this comment

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

Great document, thanks! Do you think it would be beneficial to include a practice of documenting any changes to the dependency requirements? This could include the rationale for setting or changing version ranges, providing context for future maintainers and downstream builds?

EDIT: "squash and merge" is not enabled in this repository so please do it locally and push again, thanks!

docs/dependency-management.md Outdated Show resolved Hide resolved
@dhellmann
Copy link
Contributor Author

Great document, thanks! Do you think it would be beneficial to include a practice of documenting any changes to the dependency requirements? This could include the rationale for setting or changing version ranges, providing context for future maintainers and downstream builds?

That sounds like a good follow-up for someone else to do, yes.

@dhellmann dhellmann force-pushed the requirements-management branch from 8f2dae9 to cec6bd1 Compare July 26, 2024 12:39
@dhellmann
Copy link
Contributor Author

EDIT: "squash and merge" is not enabled in this repository so please do it locally and push again, thanks!

Done

That information is useful for users and re-packagers to understand which versions of dependencies are compatible with more specificity than the ranges provide.
Tools like Dependabot will submit PRs to automatically update those pins to help us keep up with new releases of all of our dependencies.

Pinning to specific versions in the package dependencies in `pyproject.toml` or `requirements.txt` is not a good practice.
Copy link
Member

Choose a reason for hiding this comment

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

in line 16, it suggests to use pinned versions in requirements.txt for CI, but here it suggests using pinned versions in requirement.txt is not a good practice. Just found this confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You don't want to pin your package dependencies. You do want to pin your CI packages. That's done in separate files. Both tend to be named requirements.txt. The earlier text had said requirements.in but that was changed to requirements.txt.

I don't know if you're using separate requirements/constraints lists for CI, yet.

Copy link
Member

Choose a reason for hiding this comment

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

I made that suggestion assuming it was a typo - if not feel free to change it back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point of the suggestions is getting lost in the concern about specific filenames, so I've dropped the filenames and focused on how the requirements are used in different cases.

If this part is still confusing, I can just remove it all. The most important part for me right now is that the team understands their responsibility for keeping minimum versions up to date.

@dhellmann dhellmann force-pushed the requirements-management branch from cec6bd1 to c4378ce Compare July 26, 2024 15:00
@nathan-weinberg nathan-weinberg merged commit fa76420 into instructlab:main Jul 31, 2024
4 checks passed
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.

7 participants