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

Catch exceptions without JIT compiling the wrapper. #99

Merged
merged 17 commits into from
May 8, 2023

Conversation

erick-xanadu
Copy link
Contributor

@erick-xanadu erick-xanadu commented Apr 24, 2023

Context: Add a pybind wrapper to translate C++ expcetions to Python exceptions.

Description of the Change: Does not use libffi. Depends on #106.

Benefits: No compilation time spent in the wrapper. No dependency on libffi. Better UX.

@erick-xanadu erick-xanadu force-pushed the eochoa/2023-04-24/wrapper branch 10 times, most recently from 102b639 to 1f8c34c Compare April 24, 2023 22:08
@codecov
Copy link

codecov bot commented Apr 24, 2023

Codecov Report

Merging #99 (dcde822) into main (3f2d4cc) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #99      +/-   ##
==========================================
+ Coverage   98.72%   98.75%   +0.03%     
==========================================
  Files          34       35       +1     
  Lines        5565     5708     +143     
  Branches      274      274              
==========================================
+ Hits         5494     5637     +143     
  Misses         40       40              
  Partials       31       31              
Impacted Files Coverage Δ
frontend/catalyst/compilation_pipelines.py 100.00% <100.00%> (ø)
frontend/catalyst/compiler.py 100.00% <100.00%> (ø)
frontend/catalyst/jax_tracer.py 90.71% <100.00%> (ø)

... and 11 files with indirect coverage changes

@erick-xanadu erick-xanadu force-pushed the eochoa/2023-04-24/wrapper branch from a5851c8 to b95624a Compare April 24, 2023 22:30
@erick-xanadu erick-xanadu marked this pull request as ready for review April 24, 2023 22:30
@erick-xanadu
Copy link
Contributor Author

erick-xanadu commented Apr 24, 2023

With this new solution, on my machine (and therefore not a rigorous benchmark) the compilation time is back to what is expected.

real    0m33.046s
user    1m27.750s
sys     0m30.265s

Baseline (no wrapper, git revert HEAD)

real    0m34.807s
user    1m31.175s
sys     0m32.559s

Here is a link to the previous implementation:

Please note that this is not a rigorous benchmark but it roughly gives us the idea that things are staying fairly equal.

@erick-xanadu erick-xanadu marked this pull request as draft April 28, 2023 15:13
@erick-xanadu erick-xanadu force-pushed the eochoa/2023-04-24/wrapper branch 2 times, most recently from a38148a to 58e83ee Compare April 28, 2023 18:08
@erick-xanadu erick-xanadu force-pushed the eochoa/2023-04-24/wrapper branch from 58e83ee to 6ca2535 Compare May 1, 2023 12:51
@erick-xanadu erick-xanadu marked this pull request as ready for review May 1, 2023 15:52
@erick-xanadu erick-xanadu requested review from maliasadi and dime10 May 1, 2023 15:53
@erick-xanadu erick-xanadu mentioned this pull request May 2, 2023
@erick-xanadu erick-xanadu force-pushed the eochoa/2023-04-24/wrapper branch from 8850a4a to 7aa31f6 Compare May 2, 2023 16:15
.pylintrc Show resolved Hide resolved
@erick-xanadu erick-xanadu force-pushed the eochoa/2023-04-24/wrapper branch 2 times, most recently from 1c5246e to 59a4fb7 Compare May 2, 2023 19:34
.pylintrc Show resolved Hide resolved
doc/dev/installation.rst Show resolved Hide resolved
frontend/catalyst/compilation_pipelines.py Show resolved Hide resolved
frontend/catalyst/utils/wrapper.cc Outdated Show resolved Hide resolved
frontend/catalyst/utils/wrapper.cc Outdated Show resolved Hide resolved
frontend/catalyst/utils/wrapper.cc Outdated Show resolved Hide resolved
frontend/catalyst/utils/wrapper.cc Outdated Show resolved Hide resolved
frontend/test/pytest/test_compiler.py Outdated Show resolved Hide resolved
frontend/test/pytest/test_compiler.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@erick-xanadu erick-xanadu requested a review from maliasadi May 5, 2023 18:53
Copy link
Contributor

@dime10 dime10 left a comment

Choose a reason for hiding this comment

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

We'll have to wait for approval from @maliasadi but looks good to me 👍

frontend/test/pytest/test_compiler.py Outdated Show resolved Hide resolved
Copy link
Member

@maliasadi maliasadi left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants