Skip to content

Conversation

@gramalingam
Copy link
Collaborator

@gramalingam gramalingam commented Aug 26, 2024

Add utility to inline all functions in a model.

Still TODO:

  • Some edge cases to be considered for renaming and avoiding conflicts, especially with subgraphs.
  • Must ensure no variable capture happens (part of above renaming).
  • Test renaming of node names.

Fixes #1769

@gramalingam gramalingam marked this pull request as draft August 26, 2024 18:08
@codecov
Copy link

codecov bot commented Aug 26, 2024

❌ 4 Tests Failed:

Tests completed Failed Passed Skipped
12034 4 12030 2047
View the top 3 failed tests by shortest run time
onnxscript.converter_test.TestConverter test_eager_op
Stack Traces | 0.001s run time
No failure message available
onnxscript.converter_test.TestConverter test_loops_break
Stack Traces | 0.001s run time
No failure message available
onnxscript.converter_test.TestConverter test_opset_import
Stack Traces | 0.001s run time
No failure message available

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

@github-actions
Copy link

github-actions bot commented Aug 26, 2024

Test Results

     24 files  ±    0      24 suites  ±0   2h 50m 47s ⏱️ +13s
 16 877 tests +1 299  14 497 ✅ +1 296    2 339 💤 +1   35 ❌ +3   6 🔥  - 1 
354 894 runs  +   36  80 986 ✅ +   33  273 651 💤 +1  215 ❌ +3  42 🔥  - 1 

For more details on these failures and errors, see this check.

Results for commit dca5d9c. ± Comparison against base commit 74ae4cc.

This pull request skips 1 test.
tests.function_libs.torch_lib.ops_test.TestOutputConsistencyEagerCPU ‑ test_output_match_opinfo__clamp_max_cpu_float16

♻️ This comment has been updated with latest results.

@gramalingam gramalingam changed the title [DRAFT] IR-based inliner IR-based inliner Aug 30, 2024
@gramalingam gramalingam marked this pull request as ready for review August 30, 2024 22:30
@justinchuby justinchuby self-assigned this Aug 30, 2024
@justinchuby
Copy link
Collaborator

Just checking: will the meta data from the function call node be added to the inlined nodes? That would be desirable.

@gramalingam
Copy link
Collaborator Author

Just checking: will the meta data from the function call node be added to the inlined nodes? That would be desirable.

Hmmm. Yes, that would be useful. What would we do in the case of conflicts, though? (Same question comes up for rewriter, would be nice to have a common solution, if possible.)

@justinchuby
Copy link
Collaborator

In the torch exporter's case, since only nodes from the main graph have metadata written to them, taking the metadata from the main node is the most useful.

@gramalingam
Copy link
Collaborator Author

Added merging of metadata props

@gramalingam gramalingam enabled auto-merge (squash) September 5, 2024 21:17
Copy link
Collaborator

@justinchuby justinchuby left a comment

Choose a reason for hiding this comment

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

Maybe functions cannot have initialized values, so const_values are irrelevant after all?

@gramalingam gramalingam merged commit d7a6411 into main Sep 6, 2024
@gramalingam gramalingam deleted the rama/inliner branch September 6, 2024 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

Implement function inlining with the IR

3 participants