-
Notifications
You must be signed in to change notification settings - Fork 538
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
[docs] Refreshes development.md #3932
base: main
Are you sure you want to change the base?
[docs] Refreshes development.md #3932
Conversation
bjacobgordon
commented
Dec 23, 2024
•
edited
Loading
edited
- tweak confusing portions
- update stale instructions
- fix typos
- restructure to be more linear/sequential
- found while going through Turbine Camp
First PR, super stoked! 🥳 ContextAs part of Turbine Camp, I was referred to this doc to build Torch-MLIR on my Linux VM. It has a lot of potential, and I decided to nudge it in a productive direction a bunch of times as I read and used the doc. ApproachI tried to make the commits here "atomic", i.e. each one is a singular unit of work such that the file "works better"/"makes more sense" post-commit than pre-commit. The majority of the commits in this PR is the story of how I "refactored" the doc step by step to look the way it does. I wrote them with Future Us™ in mind! Each commit should have useful information of the changes made that IDEs like VSCode should be able to surface, so please don't squash haha. Ready when you are, @marbre! @marbre The only thing I'm unsure about is removing the libtorch flags |
Thanks! Marius/Jacob, please review and land as you see fit. I'll have a closer look when I'm back and can contribute any adjustments then. |
bd16189
to
0927acd
Compare
304b07f
to
a7275ff
Compare
05ea09d
to
7f23d36
Compare
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.
Thanks for working through the documentation. I certainly understand that this needs improvement and should be restructured in some way. However, I am not sure if that is the structure we should aim for. Maybe we should split out some things, like the formatting, and land this separately. Next we could iterate on the docs, maybe in smaller chunks, before restructuring to much at once. Just thinking loud and other approaches have their benefits as well.
7f23d36
to
d4b0153
Compare
2daa32c
to
ef8e61b
Compare
Okay, @marbre, I think we're almost there! How's it look now? |
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.
Just a quick review, traveling, but leaving a few comments.
docs/development.md
Outdated
|
||
Two setups are possible to build: in-tree and out-of-tree. The in-tree setup is the most straightforward, as it will build LLVM dependencies as well. | ||
- Some older pip installs may not be able to handle the recent PyTorch deps |
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.
- Some older pip installs may not be able to handle the recent PyTorch deps | |
Some older pip installs may not be able to handle the recent PyTorch deps |
I don't see the need for a bullet point here. Same at some places below.
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.
This phrase was originally a comment in the block above the shell command, very subtle. How do we communicate "you can gloss over this line"? Maybe prepending "NOTE:" instead of having the bullet point?
-DPython3_FIND_VIRTUALENV=ONLY \ | ||
-DMLIR_ENABLE_BINDINGS_PYTHON=ON \ | ||
-DLLVM_TARGETS_TO_BUILD=host \ | ||
\ |
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.
\ |
The additional line looks odd to me.
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 agree. Wanting to somehow communicate at-a-glance "this chunk of options is the key difference between the two otherwise-identical code blocks". What's another way we could do that?
cmake --build build --target check-torch-mlir | ||
```shell | ||
cmake -GNinja -Bbuild \ | ||
`# Enables "--debug" and "--debug-only" flags for the "torch-mlir-opt" tool` \ |
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.
?
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'm guessing this is not what you had in mind haha. I'll need a little more direction here, which parts seem odd?
-DPython3_FIND_VIRTUALENV=ONLY \ | ||
-DMLIR_ENABLE_BINDINGS_PYTHON=ON \ | ||
-DLLVM_TARGETS_TO_BUILD=host \ | ||
\ |
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.
\ |
1. **If you anticipate needing to frequently rebuild LLVM**, append: | ||
|
||
```shell | ||
\ |
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.
?
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.
This is what you had meant, right? All you need to get going is copying, pasting and running one of the "base" commands. Tacking this on before running is optional, right?
1. **If you need to enable local end-to-end tests**, append: | ||
|
||
```shell | ||
\ |
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.
\ |
ef8e61b
to
7825aa4
Compare
7825aa4
to
3d6994c
Compare
3d6994c
to
62b5045
Compare
Stacked onto #4048 |
62b5045
to
1fd9140
Compare
12a2a81
to
ca99afb
Compare
…ails - Makes the "in-tree" and "out-of-tree" variants adjacent to each other
…y of "in-tree" vs "out-of-tree"
…plified Build" section
- emphasizes the common options between "in-tree extended", "in-of-tree base", and "out-of-tree"
…uild using base options
- tweaks existing headers to communicate command construction rather than immediate execution - adds new header to indicate when the command should actually be executed
- was repeated 3 times!
- the use-case for this doc leans more toward "development" rather than "release"
- leverages numbering and indentation to reduce cognitive load
…fore configuring with CMake
CMake issued a warning upon fresh build: > Manually-specified variables were not used by the project: > LIBTORCH_CACHE > LIBTORCH_SRC_BUILD > LIBTORCH_VARIANT
… to numbered list
…arate code blocks - makes them individually copyable from GitHub
… Build" - makes it so that the GitHub copy button only grabs the code snippet itself
…er "Initiate Build" section
- less repetitive - emphasizes that the targets can be chained
ca99afb
to
84d17b6
Compare
Stacked onto #4081 |