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

[Relay tests] Temporary Attr Update for Order-Independent Testing #4357

Merged
merged 1 commit into from
Nov 19, 2019
Merged

[Relay tests] Temporary Attr Update for Order-Independent Testing #4357

merged 1 commit into from
Nov 19, 2019

Conversation

anijain2305
Copy link
Contributor

@anijain2305 anijain2305 commented Nov 17, 2019 •
edited
Loading

tqchen
tqchen requested changes Nov 17, 2019
View reviewed changes
src/relay/ir/op.cc Outdated
if (op_map == nullptr) {
return;
}
op_map->data_.clear();
Copy link
Member

Choose a reason for hiding this comment

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

per https://docs.tvm.ai/contribute/code_review.html#ensure-test-coverage

Need testcase for reset_attr.
The current implementation resets the attr of other ops along the current one, which is not correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I will take a look. I have misunderstood the registry codebase then.

tests/python/relay/test_pass_alter_op_layout.py Outdated
@@ -31,6 +31,17 @@ def run_opt_pass(expr, passes):
entry = mod["main"]
return entry if isinstance(expr, relay.Function) else entry.body

def reset_retrieve_alter_op_layout(op_name, alter_layout):
Copy link
Member

Choose a reason for hiding this comment

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

Consider use RAII Pattern, this will allow the code to exit correctly even if the testcase raises an exception in the middle of the body

with TempOpAttr(name, attr):
     body

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I understood what you meant. Thanks for your patience (and teaching new practices) :)

@tqchen tqchen added status: need test case need test cases to cover the change status: need update need update based on feedbacks labels Nov 17, 2019
@anijain2305 anijain2305 changed the title [Relay tests] AlterOpLayout - Temporary attr update [Relay tests][WIP] AlterOpLayout - Temporary attr update Nov 18, 2019
@anijain2305 anijain2305 force-pushed the fix_tests branch 2 times, most recently from 51f65db to 28f5f73 Compare November 18, 2019 21:29
@anijain2305 anijain2305 changed the title [Relay tests][WIP] AlterOpLayout - Temporary attr update [Relay tests] Temporary Attr Update for Order-Independent Testing Nov 18, 2019
@anijain2305
Copy link
Contributor Author

@tqchen Can you take a look now? Thanks for your patience.

tqchen
tqchen requested changes Nov 18, 2019
View reviewed changes
python/tvm/relay/testing/temp_op_attr.py Outdated

class TempOpAttr(object):
""" Temporarily changes the attr of an op. """
def __init__(self, op_name, attr_key):
Copy link
Member

Choose a reason for hiding this comment

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

document all the arguments https://docs.tvm.ai/contribute/document.html#document-python

"""
Parameters
----------

Examples
----------
"""

Copy link
Member

Choose a reason for hiding this comment

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

Can be combine the set and reset together?

# set op attrkey to value
with TempOpAttr(op_name, attr_key, attr_value):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Will update soon.

src/relay/ir/op.cc Outdated
Op op = args[0];
std::string attr_name = args[1];
auto& reg =
OpRegistry::Registry()->__REGISTER_OR_GET__(op->name).set_name();
Copy link
Member

Choose a reason for hiding this comment

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

.set_name() is not necessary.

tests/python/relay/test_ir_op.py

# Reset log fadd1 attr.
log_op = relay.op.get("log")
log_op.reset_attr("fadd1")
Copy link
Member

Choose a reason for hiding this comment

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

Let us also test TempOpAttr

@anijain2305
Copy link
Contributor Author

@tqchen comments addressed

@tqchen tqchen merged commit 26eb405 into apache:master Nov 19, 2019
@tqchen tqchen added status: accepted and removed status: need test case need test cases to cover the change status: need update need update based on feedbacks labels Nov 19, 2019
@anijain2305 anijain2305 deleted the fix_tests branch November 19, 2019 05:08
@yzhliu yzhliu mentioned this pull request Nov 22, 2019
Closed
zxy844288792 pushed a commit to zxy844288792/tvm that referenced this pull request Nov 26, 2019
@anijain2305
[Relay tests] AlterOpLayout - Temporary attr update (apache#4357)
7d041bd
yongwww pushed a commit to neo-ai/tvm that referenced this pull request Nov 26, 2019
@anijain2305 @yongwww
[Relay tests] AlterOpLayout - Temporary attr update (apache#4357)
c912a9c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tqchen tqchen tqchen approved these changes

Assignees
No one assigned
Labels
status: accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@anijain2305 @tqchen