Tutorials take 3 #267
Tutorials take 3 #267
Conversation
ci: Disable debug info in Rust binaries See merge request StanfordLegion/legion!1811
ci: Disable debug info in Rust binaries See merge request StanfordLegion/legion!1811 (cherry picked from commit 82c2ede) 227303c ci: Disable all debug info in Rust binaries to save space. Co-authored-by: Elliott Slaughter <[email protected]>
legion-ci: remove the specific tag definition for msvc jobs See merge request StanfordLegion/legion!1802
realm: fix examples and tests See merge request StanfordLegion/legion!1813
disable cuhook build by default, add option See merge request StanfordLegion/legion!1810
Realm: disable barrier broadcast See merge request StanfordLegion/legion!1809
disable cuhook build by default, add option See merge request StanfordLegion/legion!1810 (cherry picked from commit eb1ec26) f6d559c disable cuhook build by default, add option 08572ac Merge branch 'master' into sm/fix_cuhook_make Co-authored-by: Seema Mirchandaney <[email protected]>
Realm: disable barrier broadcast See merge request StanfordLegion/legion!1809 (cherry picked from commit 3eba361) 8bb9bfb realm: add broadcast kill-switch dd232c7 realm: add broadcast kill-switch 548e198 realm: remove assert 3609e63 realm: fix clang-format c33960f realm: undo cmake changes aa277d0 realm: remove empty line 487652d realm: disable fragmentation 3478e0e realm: disable broadcast_previous 539601f realm: disable tests 925078f realm: completely disable broadcast paths fe8daf5 realm: undo test changes bae24d7 realm: undo changes to runtime_impl.h ea18bec realm: fix radix 6542a35 realm: fix clang-format 166f0c2 realm: fix typo dace381 realm: undo lines 359e7c9 realm: undo namespace changes Co-authored-by: apryakhin <[email protected]>
[P0] build: Turn RDTSC off by default until we can put the PPC check back See merge request StanfordLegion/legion!1816
* cuGetProcAddress broke it's own rules for a couple of apis and kept back old versions of apis, but left cuGetProcAddress to return the newer ones, breaking source compatibility guarentees for 13.0 * For those apis that were held back, an explicit context or location was added, so modify the calls to pass in the required arguments, being compatible with older toolkits as well.
StanfordLegion#243) …ring the build. This is required for clangd, et al, to function.
Also adds the gh copilot thinking file.
Change the behavior to check the target was defined and the feature was enabled before setting it to be used/built. This is useful when the project is built as part of other projects that may have found a package but do not necessarily want that feature to be built by this project.
* Add sanitizer tests for ASAN, TSAN, and UBSAN * Unfortunately we have a leak in the python module, so disable that. * Fix a couple of found leaks * Add ucx and gasnet builds to the ci
* Fix up cuda compile args to compile SASS for everything up to the last architecture, adding ptx for the latest (fixing an issue with r580 when both sass and ptx is available for a removed architecture) * Add a REALM_DEFAULT_ARGS environment and put the default test arguments here, prepended to what is set in the test environment at runtime. This allows us to set the default, but allow the test environment to override it. * This allows us to, by default, enable GPU tests with one gpu, but override it in the test environment for more than one GPU
| * Use Machine::ProcessorQuery to enumerate processors. | ||
| * The address space shows which node (rank) the processor resides on. | ||
| */ | ||
| for(Machine::ProcessorQuery::iterator it = Machine::ProcessorQuery(machine).begin(); it; |
There was a problem hiding this comment.
Can we replace this with range-based for loops? i.e. for(Processor p : Machine::ProcessorQuery(machine)) { or something?
There was a problem hiding this comment.
Unfortunately the build instructions in the README under "try the tutorials":
cmake -B build -DREALM_BUILD_TUTORIALS=ON
do not work:
dev310 ❯ cmake -B build -DREALM_BUILD_TUTORIALS=ON
CMake Error at CMakeLists.txt:70 (project):
VERSION "$Format:%(describe:tags,match=v*)$
" format invalid.
-- Configuring incomplete, errors occurred!
Gentle reminder that I have a thimble-full of C++ expertise, and less for cmake. Perhaps best to leave actual code changes to C++ devs who can test that they work.
There was a problem hiding this comment.
@bryevdv this is usually because you either have a shallow checkout (so git describe --tags doesn't come back with a valid version tag) or you're trying to build from source with a copy of the source without git that isn't from a git archive command (i.e. a tarball from github).
The cmake build system will first try to parse the version from git. If it can't get that, it'll look at the VERSION file. The VERSION file in the repo as is needs to be expanded via the .gitattributes file which happens when you use git archive (or when github creates a source tarball).
| case Processor::LOC_PROC: | ||
| { | ||
| /** LOC_PROC: latency-optimized core (CPU), specified by -ll:cpu */ | ||
| log_app.print("Rank %u, Processor ID " IDFMT " is CPU.", p.address_space(), p.id); |
There was a problem hiding this comment.
It would be nice to replace all the log_app.print(fmt, args...) statements with log_app.print() << using the stream operator instead to show that Processor p is a printable type, you don't need to go down into p.id.
| Realm can use this model to choose the best path for computation and data movement. | ||
|
|
||
| A `Processor` can also be queried by its `kind` and the runtime supports the following options: | ||
| ## Program Setup |
There was a problem hiding this comment.
I don't understand what the program setup section is about. The markdown file highlights some includes and
"main_task()
{
"
which isn't really representing much. I think that needs to be re-imagined.
There was a problem hiding this comment.
It's "program setup" because it is the block with the first part of the program that does basic setup things. What do you want to say here? Are you suggesting to repeat "here are typical imports and a logger setup" in every file?
There was a problem hiding this comment.
If we are being specific here, the "does basic setup things" are happening inside the main function where one registers and spawns the tasks. main_task from what I see actually where the program logic is.
Are you suggesting to repeat "here are typical imports and a logger setup" in every file
I am not suggesting to repeat it...but I am suggesting that we think more about it. Perhaps that should be named "Program structure" instead and since it has the block with a name of the application task (same for events tutorial) this section could discuss what those tasks are. There is "main_task" comment in this tutorial but there is none in events tutorial.
Let me know what is your opinion since that what is important? That needs to make sense for a person who will read it afterwards presumably not knowing anything about realm.
| runs `num_tasks`, stores the events produced by `reader_task_1` into an events | ||
| vector, and combines them by calling `Event::merge_events(events).wait()`. | ||
| ## Triggering events |
There was a problem hiding this comment.
This section isn't very well aligned. We are discussing event trigering yet the code listing has some shutdown in it. Why?
There was a problem hiding this comment.
The immediately following code has to do with user events. Are you saying you want to split out the final two lines
Runtime::get_runtime().shutdown(Event::NO_EVENT, 0 /*success*/);
}
into an entirely new "shutdown" section of its own? That seems like excessively splitting the page up to me but I'll make that change if that is what you are asking for.
There was a problem hiding this comment.
Right, first shutdown can take regular events as well triggering of which users can't do. Second in this example it's not taking any event in the first place.
as long as it makes sense to you to remove it..as a person for example who reads this tutorial and doesn't necessary have to know realm in advance..is this a well suited code listing for this section?
| owner node, and remote waiters are other nodes that are interested in the | ||
| `Event` (event dependencies). | ||
|
|
||
| ## Program setup |
There was a problem hiding this comment.
I think the program setup section is better aligned in hello world tutorial as it discusses actual setup we are doing such as creating loggers etc. In this tutorial we are just plainly pasting some listing. Perhaps this section should talk about the tasks that the program is about to run.
|
What is the status of this PR? |
|
I was out all last week, and I am currently occupied with cupynumeric work. As I mentioned, I'm neither a C++ dev or a realm SME so it might be quicker for someone else to push this to conclusion, since the comments relate to the content rather than docs infra or automation (i.e where I might be more useful). |
12 participants
Third time is the charm?