Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding tt-metalium #28854
base: main
Are you sure you want to change the base?
Adding tt-metalium #28854
Changes from 23 commits
aad8e55
88b02fd
4cbe34f
43ca25a
6fb0f13
d0dada2
b935312
9bcf248
f7a7349
605a5f8
5d06a8c
8ccf7fc
dc68535
d1b88a2
f522568
e858c4b
2d3f737
bbc7d92
04d3263
ff638a1
8678be7
293f3f7
65387d1
9842d1d
163f19b
2a3032d
ed3b681
3246ece
de4c472
9924066
37eb9c7
a5f7a46
4902999
29beb3e
f981c08
60c4a31
e41eca4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is there a reason this is using an RC (
rc13
)?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.
I am putting a lot of fixes into main to support this effort. I needed a very recent version of the code. We upload release candidates daily. We cut releases very rarely.
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.
In the installed files I see
and a whole lot more along those lines. It seems to me that this shouldn't be under
site-packages
(which is python-specific), unlesstt-metalium
depends on exactly that layout?Similarly for
lib/python3.10/site-packages/tt_metal
, which looks to be just full of.h
&.cpp
files. Why not put all that into a separate output that install these things into$PREFIX/{include,lib,...}
, and then depend on that intt-metalium
(again, we could make a symlink to the location in$SP_DIR
if necessary).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.
Sadly, we need many such files to support JIT compile at runtime.
The project depends on the current structure.
For PyPi we produce a wheel with all necessary files. (There is pending work to trim the number of files).
I don't want Anaconda's behavior to differ from normal pip / python behavior.
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.
That's OK, but symlinks will still work for this.
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.
@h-vetinari I am not a fan of what you are asking me to do, because it makes the Anaconda packaging diverge from our existing packaging for Pypi. The C++ sources that are packaged are not to be compiled by the host, so they don't really belong in conda environment's include/
It would be quite a bit of work to break out all non-python sources into different outputs, and for no functional benefit. I would also have to create a patch file, to change the existing setup.py driven packaging.
I pushed two commits tonight to start giving things a try. Let me know if thats what you were imaginging.
My request to you, is lets just not do this for now? We can work on improving our python packaging and distribution scheme and port the upstream improvements to this recipe in time?
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.
UPDATE: I reverted the commits, because the build was failing.
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.
I confirm I am willing to be listed here. ✅