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

Compile time #154

Merged
merged 42 commits into from
Jan 24, 2025
Merged

Compile time #154

merged 42 commits into from
Jan 24, 2025

Conversation

DrTodd13
Copy link
Collaborator

@DrTodd13 DrTodd13 commented Jan 15, 2025 •
edited
Loading

Changes included in this PR

Cache many internal njit and overload methods. Add support for caching of string-generated functions.

Testing strategy

Need to understand better how caching is tested in Bodo and how that applies to internal functions.

User facing changes

None.

Checklist

  • Pipelines passed before requesting review. To run CI you must include [run CI] in your commit message.
  • I am familiar with the Contributing Guide
  • I have installed + ran pre-commit hooks.

@DrTodd13 DrTodd13 marked this pull request as draft January 15, 2025 23:40
@codecov Codecov
Copy link

codecov bot commented Jan 16, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 83.72828% with 103 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@9178dad). Learn more about missing BASE report.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #154   +/-   ##
=======================================
  Coverage        ?   77.79%           
=======================================
  Files           ?      169           
  Lines           ?    61900           
  Branches        ?     8663           
=======================================
  Hits            ?    48158           
  Misses          ?    11648           
  Partials        ?     2094

ehsantn
ehsantn reviewed Jan 21, 2025
View reviewed changes
Todd A. Anderson added 22 commits January 21, 2025 09:54
Add Numba CacheLocator for string-generated functions.
89564c3
Add cache flag for njit and overloads.
2514414
Add caching options to overloads, njits, and convert string-generated…
48e0c39 
… functions to be cacheable.
[run CI]
f569f6f
Remove a print that I don't know how it got into the commit.
414942c
Hash whole function inside bodo_exec. Append hash to function name. T…
b67be9b 
…his precludes recursion at the moment. Get __name__ from globals() so eliminate that unneeded arg.
[run CI]
c90bcd1
More conversion of exec to bodo_exec. Additional overload_method cach…
d936c8a 
…e options.
[run CI]
7dbec7d
Remove from problematic caching.
3c4dce6
[run CI]
9372a80
Rename impl.
ebc5911
Convert exec to bodo_exec.
60428eb
[run CI]
1e9e883
Convert exec to bodo_exec.
d7f94fc
Rename impl to bodo_* because those functions go through _gen_init_df…
9a31cc2 
… which now uses bodo_exec.
[run CI]
0efc535
Fix bug in bodo_exec for nested functions. re-enable series_clip cach…
0702e27 
…ing.
More conversion of exec to bodo_exec. Additional overload_method cach…
1ed0a11 
…e options.
[run CI]
cccc4ba
Remove from problematic caching.
06aac30
[run CI]
9fa09c2
Todd A. Anderson added 7 commits January 21, 2025 10:13
[run CI]
b8b8483
Turn off caching for dataframe_astype due to weird pickling error abo…
3d21f67 
…ut get_rank.
[run CI]
193d9bf
Remove all __pycache__ from subdirs of bodo. After first test run, ma…
2265f9c 
…ke sure that the string-generated bodo function cache is populated.
Use bodo_exec for string-generated func. Rename some internal funcs a…
b1ce6dd 
…way from 'impl'.
Add a function to test in a subprocess two times right after all the …
ba82390 
…caches are cleared to make sure the second run hits the internal cache of a function known to be called.
[run CI]
c80a50e
@DrTodd13 DrTodd13 marked this pull request as ready for review January 23, 2025 00:09
@DrTodd13 DrTodd13 requested review from ehsantn and srilman January 23, 2025 00:10
ehsantn
ehsantn reviewed Jan 23, 2025
View reviewed changes
Copy link
Collaborator

@ehsantn ehsantn left a comment

Choose a reason for hiding this comment

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

Thanks @DrTodd13. Looks good to me overall. Just a comment about moving the test to a separate file.

srilman
srilman approved these changes Jan 23, 2025
View reviewed changes
Copy link
Contributor

@srilman srilman left a comment

Choose a reason for hiding this comment

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

Overall LGTM. I'm assuming you're going to add the call_id = create_arg_hash(... to the other custom IR nodes like the Iceberg, SQL, Join ... in a followup. Will let Ehsan review the compiler stuff

@DrTodd13 DrTodd13 merged commit c588686 into main Jan 24, 2025
22 checks passed
@DrTodd13 DrTodd13 deleted the compile_time branch January 24, 2025 17:52
@ehsantn ehsantn mentioned this pull request Jan 26, 2025
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehsantn ehsantn ehsantn approved these changes

@srilman srilman srilman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DrTodd13 @ehsantn @srilman