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.

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

[WIP] replace new compiler driver by calling compiler-cli using subprocess #1285

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

Conversation

mehrdad2m
Copy link
Contributor

@mehrdad2m mehrdad2m commented Nov 6, 2024

Context: The old compiler driver api which uses the pybind11 as an interface between the python frontend and the compiler should be replaced with the new CLI tool (catalyst-cli).

Description of the Change:
Removes the pybind11 interface in favour of calling catalyst-cli using python subprocess.

Benefits:
the catalyst-cli will be completely independent of the python frontend and consequently, compiler symbols will be separated from the frontend

Possible Drawbacks:
Using subprocess requires us to connect with the catalyst-cli using files on disk. Therefore we need to save the IR to a file temporarily and parse it again in the compiler which can be problematic when IR size grows. This should be improved in future using interprocess communication.

[sc-73537]
[sc-73538]

@mehrdad2m mehrdad2m marked this pull request as ready for review November 7, 2024 20:07
@erick-xanadu erick-xanadu added the author:build-wheels Run the wheel building workflows on this Pull Request label Nov 7, 2024
Makefile Show resolved Hide resolved
Copy link
Contributor

@erick-xanadu erick-xanadu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also test locally and not have to wait for CI/CD by typing "make wheel" and then pip installing the wheel in your local machine and then removing the build directories and running the tests. The tests should pass as catalyst-cli should now be available on the wheel itself as long as the path is correct.

@mlxd
Copy link
Member

mlxd commented Nov 11, 2024

Hey @mehrdad2m

Removes the pybind11 interface in favour of calling catalyst-cli using python subprocess.

Why is this preferred? Using the Python extension module should be the way to go about this no? Everything lives in the same process space, no need to share data/clean up directories/no need for IPC.

@mehrdad2m
Copy link
Contributor Author

Hey @mehrdad2m

Removes the pybind11 interface in favour of calling catalyst-cli using python subprocess.

Why is this preferred? Using the Python extension module should be the way to go about this no? Everything lives in the same process space, no need to share data/clean up directories/no need for IPC.

Hi @mlxd,

Performance wise, you are definitely right. However one of the main objectives of this story (@erick-xanadu please feel free to elaborate if I miss something) is to detach the address space of the compiler form the python frontend so we avoid any symbol clash.
The solution so far was to hide llvm symbols but we can not do that anymore if we want to use llvm plugins. Another alternative that was explored was the multiprocessing library in python, but we found it to be too slow as it imports all the libraries once a new python interpreter is spawned. So we pretty much arrived at the subprocess solution by a process of elimination.

@mlxd
Copy link
Member

mlxd commented Nov 11, 2024

Hey @mehrdad2m

Removes the pybind11 interface in favour of calling catalyst-cli using python subprocess.

Why is this preferred? Using the Python extension module should be the way to go about this no? Everything lives in the same process space, no need to share data/clean up directories/no need for IPC.

Hi @mlxd,

Performance wise, you are definitely right. However one of the main objectives of this story (@erick-xanadu please feel free to elaborate if I miss something) is to detach the address space of the compiler form the python frontend so we avoid any symbol clash. The solution so far was to hide llvm symbols but we can not do that anymore if we want to use llvm plugins. Another alternative that was explored was the multiprocessing library in python, but we found it to be too slow as it imports all the libraries once a new python interpreter is spawned. So we pretty much arrived at the subprocess solution by a process of elimination.

Thanks for the context @mehrdad2m

@erick-xanadu
Copy link
Contributor

@mlxd , happy to document everything in the shortcut ticket and/or discuss offline :)

Copy link

codecov bot commented Nov 12, 2024

Codecov Report

Attention: Patch coverage is 96.92308% with 2 lines in your changes missing coverage. Please review.

Project coverage is 98.37%. Comparing base (fb8f93f) to head (d9d9ad7).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
frontend/catalyst/compiler.py 96.07% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1285      +/-   ##
==========================================
+ Coverage   97.95%   98.37%   +0.41%     
==========================================
  Files          77       78       +1     
  Lines       11321    11361      +40     
  Branches      982      990       +8     
==========================================
+ Hits        11090    11176      +86     
+ Misses        181      127      -54     
- Partials       50       58       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@erick-xanadu erick-xanadu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just small changes.

frontend/catalyst/utils/runtime_environment.py Outdated Show resolved Hide resolved
frontend/catalyst/compiler.py Outdated Show resolved Hide resolved
Copy link
Contributor

@erick-xanadu erick-xanadu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll repeat what I commented offline: I think this PR is ready to be merged. It would also be good to benchmark the compilation time. Seeing this difference between the implementations would help prioritize the need for IPC next quarter. Let's try and benchmark before merging, I know there are other stories depending on this so if it takes too long to set up the benchmarks, we can discuss offline and approve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author:build-wheels Run the wheel building workflows on this Pull Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants