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.

Sign up for GitHub

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

Jump to bottom

Get use- and colorful output from compile.sh #172

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Get use- and colorful output from compile.sh #172

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
012e9bd
used script wrapper to capture colored output of compile command
JBludau Feb 9, 2023
c1be48b
changed check for compiled module to check for .so file
JBludau Mar 13, 2023
d24a371
corrected empty argument to be really empty and not a single whitespace
JBludau Mar 13, 2023
d471960
restricted colored output to linux machines for now
JBludau Mar 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions pykokkos/core/cpp_setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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":
Copy link
Contributor

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.

#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) + "\""
Copy link
Contributor

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?)


compile_result = subprocess.run(command, cwd=output_dir, capture_output=True, check=False, shell=True)
Copy link
Contributor

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 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'")
Copy link
Contributor

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?

sys.exit(1)

patchelf: List[str] = ["patchelf",
Expand Down Expand Up @@ -323,10 +329,14 @@ def get_cuda_compute_capability(self, compiler: str) -> str:
@staticmethod
def is_compiled(output_dir: Path) -> bool:
"""
Check if an entity is compiled
Check if an entity is compiled. It does this via checking if any .so file exists.

:param output_dir: the directory containing the compiled entity
:returns: true if compiled
:returns: true if any .so object exists
"""
so_files = output_dir.glob("*.so")
contains_so_files = False
for file in so_files:
contains_so_files = file.is_file()
Comment on lines +337 to +340
Copy link
Contributor Author

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...


return output_dir.is_dir()
return contains_so_files