Modify the compile parameter in baseline benchmarks to executor
#3350
Conversation
… iterate through test items once
| benchmark_fn = { | ||
| "eager": bcast_add_fwd_fn, | ||
| "torchcompile": torch.compile(bcast_add_fwd_fn), | ||
| } |
There was a problem hiding this comment.
Personally I'd avoid using pure strings as much as possible because it'd be difficult to debug when things don't work as intended. For example, I suppose you won't get any error even if you typed "torch.compile".
Not a request to change anything, but just my two cents.
There was a problem hiding this comment.
we 've seen a similar typo err where device was typed as degvice
There was a problem hiding this comment.
Agreed, I considered using enums, but the misspelling can cause errors with that too.
Here, if torchcompile is spelled as torch.compile, in later lines, benchmark_fn[executor] will error out. So combined with the assertion that @liqiangxl suggested in the below comment for get_test_executor, we should be able to catch any spelling errors.
Please let me know if you have any suggestions which may be more error-proof.
| def get_test_executor(item) -> str | None: | ||
| if hasattr(item, "callspec") and "executor" in item.callspec.params: | ||
| return item.callspec.params["executor"] | ||
| return None |
There was a problem hiding this comment.
suggest adding a check ensure executor is one of "eager", "torchcompile", "thunder" to avoid typo, e.g. "torchcompile" -> "torch.compile"
|
LGTM, can put a note about the design to the original issue? |
|
!build |
This PR is the first step in adding
thunder.jitbenchmarks.The major change is modifying the
compileparameter toexecutorwith valueseager,torchcompile,thunder. This PR does not introduce any new thunder benchmarks (to be done in next PR).CC: @xwang233 for dashboard changes.
Issue: #2718