-
Notifications
You must be signed in to change notification settings - Fork 20
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
Get use- and colorful output from compile.sh
#172
base: main
Are you sure you want to change the base?
Conversation
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.
Looks like the CI is failing for the pre-compilation shims used for array API conformance.
Ignoring that, I tried the problem workunit I described in gh-168 with and without this patch, but I ended up with the same error message because it seems to be a runtime error that isn't capable of providing the actual C++ line that overflows or whatever?
On branch treddy_arange
with this reproducer test:
import pykokkos as pk
def main():
pk.arange(0, 2147483648, 100000, dtype=pk.uint32)
if __name__ == "__main__":
main()
Before/after I get this:
File "/home/tyler/github_projects/pykokkos/test.py", line 9, in <module>
main()
File "/home/tyler/github_projects/pykokkos/test.py", line 5, in main
pk.arange(0, 2147483648, 100000, dtype=pk.uint32)
File "/home/tyler/github_projects/pykokkos/pykokkos/lib/create.py", line 62, in arange
_ufunc_kernel_dispatcher(tid=size,
File "/home/tyler/github_projects/pykokkos/pykokkos/lib/ufuncs.py", line 50, in _ufunc_kernel_dispatcher
ret = sub_dispatcher(tid, desired_workunit, **kwargs)
File "/home/tyler/github_projects/pykokkos/pykokkos/interface/parallel_dispatch.py", line 175, in parallel_for
func(**args)
RuntimeError: Unable to cast Python instance of type <class 'int'> to C++ type 'int'
terminate called after throwing an instance of 'std::runtime_error'
what(): Kokkos allocation "" is being deallocated after Kokkos::finalize was called
Aborted (core dumped)
Let me see if I can dig up a compilation-specific issue that didn't have a good error message.
Well, I don't have another compilation failure case handy at the moment, but another problem error message scenario is a failed translation like the one in gh-161. For example, doing this on latest index 89b190c..31ba284 100644
--- a/pykokkos/lib/ufunc_workunits.py
+++ b/pykokkos/lib/ufunc_workunits.py
@@ -3,6 +3,7 @@ import pykokkos as pk
@pk.workunit
def exp_impl_1d_double(tid: int, view: pk.View1D[pk.double], out: pk.View1D[pk.double]):
+ mod: float
out[tid] = exp(view[tid])
Produces a huge mess of output, some of which is sent to
Obviously, that output doesn't change with this patch since it is a translation issue. |
hmm, this looks like a runtime error thrown by bounds checking before casting python variables into c++ types. It looks like even though you requested |
hmm this looks like we try to load a |
so_files = output_dir.glob("*.so") | ||
contains_so_files = False | ||
for file in so_files: | ||
contains_so_files = file.is_file() |
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 now checks for ANY .so file in the path. Timing with a 1000 iterations shows, that asking if the dir exists has the same cost as checking for the .so file (if only one is there). This also triggers a recompile if the .so was not found. And if the recompile fails gives the instructions for the colored compiler output.
Nevertheless, we do not check for the name, as this would require a larger refactor (the method could no longer be static or we would need to pass in the module name from outside). For the different backends we should be fine, as the backend name is contained in the folder path. But we might still need the module names, as we have this multigpu thing where multiple .so files with different names exist in the same folder. But someone first using only one gpu and then multiple without clearing the cache might be an edge case we do not care about...
@tylerjereddy could you retry if this gives you a compilation error? If not we might need to generate a hash from the generated source we send to the compiler after translation in order to check if the source has changed between runs ... this would be a major redesign thing |
@@ -243,11 +243,17 @@ def invoke_script(self, output_dir: Path, space: ExecutionSpace, enable_uvm: boo | |||
compute_capability, # Device compute capability | |||
lib_suffix, # The libkokkos* suffix identifying the gpu | |||
str(compiler_path)] # The path to the compiler to use | |||
compile_result = subprocess.run(command, cwd=output_dir, capture_output=True, check=False) | |||
|
|||
if sys.platform == "linux" or sys.platform == "linux2": |
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 should always just be linux
for any version of Python we support now.
|
||
if compile_result.returncode != 0: | ||
print(compile_result.stderr.decode("utf-8")) | ||
print(f"C++ compilation in {output_dir} failed") | ||
print(f"C++ compilation in {output_dir} failed. For colored compiler output (on linux) run 'cat {output_dir}/compile.out'") |
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.
Why are we printing instead of using Python error classes/handling here?
command = [string if string != '' else "''" for string in command] | ||
command: Str = "script --log-io compile.out --return --command " + "\""+" ".join(command) + "\"" | ||
|
||
compile_result = subprocess.run(command, cwd=output_dir, capture_output=True, check=False, shell=True) |
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.
For the mod: float
example above, I see no improvement on this branch:
Translation of <ast.FunctionDef object at 0x7f53c88238b0> failed
Which failure exactly do you want me to try on develop
vs. this branch?
if sys.platform == "linux" or sys.platform == "linux2": | ||
#on linux we can color the output otherwise this is not implemented | ||
command = [string if string != '' else "''" for string in command] | ||
command: Str = "script --log-io compile.out --return --command " + "\""+" ".join(command) + "\"" |
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 think I'm a bit confused--can't we just use standard Python error handling/output machinery to form a nicely-constructed error without requiring a command-line callout? If we really need color there is portable stuff like https://github.com/Textualize/rich, but first I want to know which errors I should compare on develop
vs. this branch to see an improvement? (i.e., do you have a reproducer I can try?)
This originated from a discussion with @tylerjereddy on slack.
In short: We would like to have a better error handling if stuff does not compile. Ideally we want to even have warnings before the compilation stage fails but this is a goal for the future. As a starter I like to propose that we capture the output of the compilation script as good as we can (this includes warnings and errors in the right order and colors if the compiler does a colored and highlighted output).
Apparently scripts detect if the output is piped into a file or a shell and restrict the coloring/hinting if it goes to a file. The linux standard program
script
can run a command and force the output to be logged as if it were in a shell thus capturing the coloring and other highlights.If I artificially introduce an error in the generated cpp file I get something like this when running a pykokkos script that compiles the broken cpp file:
When pasting the newly written
.out
file with the suggested command I get this rendered in my terminal:Which is the same as I would get from directly running the compile line on the generated .cpp file