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

[MetaSchedule] Apply-History-Best Task Filtering #11692

Conversation

junrushao
Copy link
Member

This PR enables task filtering in Apply-History-Best, which is used in
Relay/Relax integration. Previously, even though a task is ruled out
during task extraction, it still shows up in Relay compilation due to
the lack of filtering on Apply-History-Best. However, TE-to-TIR
conversion te.CreatePrimFunc doesn't support all cases with hybrid
operators involved, which leads to post-tuning failure affecting
multiple models.

@junrushao junrushao marked this pull request as ready for review June 13, 2022 17:28
@junrushao
Copy link
Member Author

CC: @zxybazh @csullivan

stack.pop_back();
if (tensor->op->IsInstance<PlaceholderOpNode>()) {
// do nothing
} else if (tensor->op->IsInstance<ComputeOpNode>()) {
Copy link
Contributor

@csullivan csullivan Jun 13, 2022

Choose a reason for hiding this comment

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

Suggested change
} else if (tensor->op->IsInstance<ComputeOpNode>()) {
} else if (tensor->op->IsInstance<ComputeOpNode>() || tensor->op->IsInstance<ExternOpNode>()) {

You'll need to check for extern ops here as well (ref).

Copy link
Member Author

@junrushao junrushao Jun 14, 2022

Choose a reason for hiding this comment

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

This is certain an interesting design tradeoff. MetaSchedule/AutoScheduler default rules are not supposed to handle all the ExternOp and HybridOp in every model, because they are already scheduled (e.g. the NMS operator), or do not need to be scheduled (e.g. external library). Therefore, we are filtering out all those operations by default to make sure MetaSchedule doesn't break on customer models.

Alternatively, given we support customized filtering methods, do you think it's a good idea if we expose another method that doesn't rule out extern-op for customized usecases?

This PR enables task filtering in Apply-History-Best, which is used in
Relay/Relax integration. Previously, even though a task is ruled out
during task extraction, it still shows up in Relay compilation due to
the lack of filtering on `Apply-History-Best`. However, TE-to-TIR
conversion `te.CreatePrimFunc` doesn't support all cases with hybrid
operators involved, which leads to post-tuning failure affecting
multiple models.
@junrushao junrushao force-pushed the feature/2022-06-13/apply-history-best-filtering branch from 630c605 to 2f21055 Compare June 14, 2022 18:03
target,
params,
schedule_fn,
te_filter_func="meta_schedule.DefaultTaskFilterAllowExtern",
Copy link
Member Author

@junrushao junrushao Jun 14, 2022

Choose a reason for hiding this comment

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

@csullivan there are a couple of options in my mind for us to unbreak MS's current workflow with customer models:

  • use te_filter_func when we want to specifically allow some te.extern operation;
  • use a different te.extern_prim_func to direct the default task filter to accept them by default
  • use annotations to instruct the default task filter instead

let me know what you think!

Copy link
Contributor

Choose a reason for hiding this comment

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

@junrushao1994 thanks for the discussion! I think the approach you have taken with a separate filter function is a good solution for now.
From the integration side I think it could indeed be desirable to separate out the functionality into a new TE Op (your second option I believe) that can be included by the default filter func so that it is transparent to end user. But for now I think it's perfectly fine to approach it when the need arises. Thanks!

@Hzfengsy Hzfengsy merged commit 1312658 into apache:main Jun 15, 2022
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