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

adding the trace-compile option to the julia initialization #530

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

rdyro
Copy link

@rdyro rdyro commented May 5, 2023

It'd be nice to be able to trace the compilation performed by Julia called via PyJulia. The main use case is for the precompilation of a Julia library triggered primarily through their Python interface.

The change is a single line adding the trace-compile option.

@mkitti
Copy link
Member

mkitti commented May 6, 2023

Thanks. Could you work on updating the tests?

@rdyro
Copy link
Author

rdyro commented May 6, 2023

I'll take a look and push another commit soon, thanks!

@codecov
Copy link

codecov bot commented May 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (69b734f) 85.22% compared to head (7bd924f) 87.60%.
Report is 16 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #530      +/-   ##
==========================================
+ Coverage   85.22%   87.60%   +2.37%     
==========================================
  Files          39       40       +1     
  Lines        2342     2364      +22     
==========================================
+ Hits         1996     2071      +75     
+ Misses        346      293      -53     

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

@rdyro
Copy link
Author

rdyro commented May 7, 2023

I fixed all errors in existing tests and I added a new test specifically for the new trace_compile option.

If you can take a look now, that'd be great. @mkitti

Copy link
Collaborator

@MilesCranmer MilesCranmer left a comment

Choose a reason for hiding this comment

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

Looks good. I have one suggestion for the test to make sure it's actually working (and not just printing random stuff to the file). Sorry for taking so long to look at this!

)
)
assert (trace_compile_path).exists()
assert len(trace_compile_path.read_text().strip()) > 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there some compilation string that you know should be printed to this file? If so, could you test that it actually shows up there?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that's a great idea! I added a check for the precompilation directive of the sin function we invoke to test precompilation.

@rdyro
Copy link
Author

rdyro commented Jan 17, 2024

I think with the additional test, this should look better now. Thanks for taking a look @MilesCranmer

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