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

Move requirements-iree-*.txt to top level. #765

Merged
merged 3 commits into from
Jan 7, 2025

Conversation

ScottTodd
Copy link
Member

This is prep work for #760.

I also considered putting the files under build_tools/ or shark-ai/, but we already have a few requirements files in the repository root. Still not as many as https://github.com/vllm-project/vllm though 😛.

Copy link
Collaborator

@marbre marbre left a comment

Choose a reason for hiding this comment

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

The failing workflow either pull in unpinned versions or if using the pinned version at least install what is expected, thus the patch does what it should.

I am still not 100% convinced as the requirements-iree-compiler.txt was supposed to keep optional (!) dependencies for shortfin only and if a user wants to install those, at least I would expect or look in the shortfin subdir. However, for what we aim for right now this is probably a huge improvement even though we might iterate later.

Comment on lines -46 to +49
# Pins
# Keep in sync with requirements-iree-compiler.txt.
# Version pins for dependencies.
# Prefer to keep the IREE git tag synced with the Python package version in the
# requirements-iree-pinned.txt file. At a minimum, the compiler from those
# packages must be compatible with the runtime at this source ref.
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI @marbre I added on to the comment here

@ScottTodd
Copy link
Member Author

I am still not 100% convinced as the requirements-iree-compiler.txt was supposed to keep optional (!) dependencies for shortfin only and if a user wants to install those, at least I would expect or look in the shortfin subdir. However, for what we aim for right now this is probably a huge improvement even though we might iterate later.

How about having a requirements-iree-unpinned.txt and requirements-iree-pinned.txt in both shortin/ and sharktank/? I'm expecting that we'll keep iterating a bit. Might just start with this PR as-is.

@ScottTodd ScottTodd merged commit 0e0b42a into nod-ai:main Jan 7, 2025
25 of 28 checks passed
@ScottTodd ScottTodd deleted the requirements-iree-move branch January 7, 2025 17:11
@marbre
Copy link
Collaborator

marbre commented Jan 7, 2025

How about having a requirements-iree-unpinned.txt and requirements-iree-pinned.txt in both shortin/ and sharktank/? I'm expecting that we'll keep iterating a bit. Might just start with this PR as-is.

Seems my comment got lost (?). My concern might be not worth it as we don't pull in requirements via requirements files in our wheel (we rather have it in the pyproject.toml files). It what more like to give background on the initial motivation. I think we should keep it easy and lightweight and go with what you have. If we really want to split it up, we can do that in a follow up.

monorimet pushed a commit that referenced this pull request Jan 8, 2025
This is prep work for #760.

I also considered putting the files under `build_tools/` or `shark-ai/`,
but we already have a few requirements files in the repository root.
Still not as many as https://github.com/vllm-project/vllm though 😛.
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