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

Set sharktank package deps during package build reliably. #440

Merged
merged 11 commits into from
Nov 8, 2024

Conversation

ScottTodd
Copy link
Member

@ScottTodd ScottTodd commented Nov 7, 2024

Progress on #294.

We could mirror requirements between requirements.txt and pyproject.toml or keep them dynamic. For packaging, we probably don't want to set explicit versions unless absolutely required. Developers may want to pin to a specific version to make testing consistent though.

As part of this I added a dep on iree-turbine to sharktank's requirements.txt and then switched workflows and user guides to install sharktank first and then install a newer/local version from git if desired.

We could mirror these between requirements.txt and pyproject.toml or keep them dynamic. For packaging, we don't want to set explicit versions unless absolutely required. Developers may want to pin to a specific version to make testing consistent.
@ScottTodd ScottTodd requested a review from marbre November 7, 2024 00:45
sharktank/requirements.txt Outdated Show resolved Hide resolved
Copy link
Member Author

Choose a reason for hiding this comment

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

We could mirror these between requirements.txt and pyproject.toml or keep them dynamic. For packaging, we don't want to set explicit versions unless absolutely required. Developers may want to pin to a specific version to make testing consistent.

@stellaraccident opinions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Industry guidance:

@@ -1,7 +1,12 @@
iree-turbine

# Runtime deps.
gguf==0.6.0
numpy==1.26.3
Copy link
Member Author

Choose a reason for hiding this comment

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

I ran into errors trying to unpin these versions (replacing == with >=). Sample logs: https://github.com/nod-ai/SHARK-Platform/actions/runs/11714432277/job/32629101709#step:6:64

Maybe one of the deps doesn't support numpy 2.0? I can play dependency-updates-whack-a-mole to try unpinning some of the packages I guess...?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably need to, yes :/ Can help with that on Monday. Should probably fill a follow up issue to keep track.

@ScottTodd ScottTodd marked this pull request as ready for review November 8, 2024 00:41
@@ -1,7 +1,12 @@
iree-turbine
Copy link
Member Author

Choose a reason for hiding this comment

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

As part of this I added a dep on iree-turbine to sharktank's requirements.txt and then switched workflows and user guides to install sharktank first and then install a newer/local version from git if desired.

@stellaraccident is this set of changes okay?

sharktank/README.md Outdated Show resolved Hide resolved
sharktank/README.md Outdated Show resolved Hide resolved
@@ -1,7 +1,12 @@
iree-turbine

# Runtime deps.
gguf==0.6.0
numpy==1.26.3
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably need to, yes :/ Can help with that on Monday. Should probably fill a follow up issue to keep track.

@ScottTodd ScottTodd force-pushed the sharktank-deps-package branch from 887fb77 to 7ee48ae Compare November 8, 2024 18:59
@ScottTodd ScottTodd merged commit 9d724a2 into nod-ai:main Nov 8, 2024
5 checks passed
@ScottTodd ScottTodd deleted the sharktank-deps-package branch November 8, 2024 19:15
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.

2 participants