Skip to content

[BugFix][UMA] Bugfix of target attr and fixed link in tutorial #12731

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
Closed
wants to merge 1 commit into from
Closed

[BugFix][UMA] Bugfix of target attr and fixed link in tutorial #12731

wants to merge 1 commit into from

Conversation

MichaelJKlaiber
Copy link
Contributor

There was a flaw in UMA backend.py that triggered an error when for passing target attributes. It is fixed.

A link in the UMA tutorial was broken and fixed by this PR.

@MichaelJKlaiber
Copy link
Contributor Author

@cgerum @areusch

lhutton1
lhutton1 reviewed Sep 8, 2022
View reviewed changes
python/tvm/relay/backend/contrib/uma/backend.py
@@ -278,7 +278,7 @@ def register(self) -> None:
"""
registration_func = tvm.get_global_func("relay.backend.contrib.uma.RegisterTarget")

for name, attr in self._target_attrs:
for name, attr in self._target_attrs.items():
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the fix @MichaelJKlaiber, just curious why this didn't get caught by https://github.com/apache/tvm/blob/main/tests/python/contrib/test_uma/test_target.py#L68?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe because this was too generic (though I expect that to be some sort of TypeError) :

tvm/tests/python/contrib/test_uma/test_target.py

Line 72 in 2aa0d1f

with pytest.raises(ValueError):
.

Should we make sure that we catch the correct exception ?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to @manupak's suggestion; can we either use TVMError or a subclass of that?

@MichaelJKlaiber
Copy link
Contributor Author

@areusch, @manupak , what are your 2cents here?

This seems like a minor fix.

manupak
manupak reviewed Sep 14, 2022
View reviewed changes
Copy link
Contributor

@manupak manupak left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

I agree with @lhutton1 here. It is puzzling as to why we were not seeing it there. Maybe specializing the ValueError to make sure we catch the right one with the description "Target attribute None is not supported." might be the solution. WDYT ?

python/tvm/relay/backend/contrib/uma/backend.py
@@ -278,7 +278,7 @@ def register(self) -> None:
"""
registration_func = tvm.get_global_func("relay.backend.contrib.uma.RegisterTarget")

for name, attr in self._target_attrs:
for name, attr in self._target_attrs.items():
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe because this was too generic (though I expect that to be some sort of TypeError) :

tvm/tests/python/contrib/test_uma/test_target.py

Line 72 in 2aa0d1f

with pytest.raises(ValueError):
.

Should we make sure that we catch the correct exception ?

@areusch
Copy link
Contributor

areusch commented Sep 15, 2022

thanks @MichaelJKlaiber ! agreed with the others that it would be great if we could update the test to cover this case as well

@fpedd fpedd mentioned this pull request Oct 17, 2022
Closed
21 tasks
@areusch areusch added needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it and removed needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it labels Oct 19, 2022
@cbalint13
Copy link
Contributor

cbalint13 commented Dec 12, 2022 •
edited
Loading

@MichaelJKlaiber

  • The bug annoyed UMA initialisation, thanks for the fix !

@areusch , @manupak, @lhutton1

  • LGTM, confirming that in real practice the issue is fixed.

Can this be moved forward and merged ?

@cbalint13 cbalint13 mentioned this pull request Dec 15, 2022
Merged
lhutton1 pushed a commit that referenced this pull request Dec 16, 2022
@cbalint13
[BugFix][UMA] Protect target registration (#13624)
0eabbac 
This PR address fixes for UMA target registration.
* Fix the doc issue #13304 
* Continues stalled PR #12731 

Changes:
* Incorporates all proposed fixes from mentioned [PR #12731](#12731)
* Address test case concerns and discussions from [PR #12731](#12731)
* **NEW:** Already exiting target cannot be created, explicit error on this.
* **NEW:** Attributes having special/reserved scope cannot be created explicitly.

It also address proper test cases for all the above.
@lhutton1
Copy link
Contributor

Closing as this PR is superseded by #13624

@lhutton1 lhutton1 closed this Dec 16, 2022
leandron pushed a commit to leandron/tvm that referenced this pull request Jan 19, 2023
@cbalint13 @leandron
[BugFix][UMA] Protect target registration (apache#13624)
da30a4a 
This PR address fixes for UMA target registration.
* Fix the doc issue apache#13304
* Continues stalled PR apache#12731

Changes:
* Incorporates all proposed fixes from mentioned [PR apache#12731](apache#12731)
* Address test case concerns and discussions from [PR apache#12731](apache#12731)
* **NEW:** Already exiting target cannot be created, explicit error on this.
* **NEW:** Attributes having special/reserved scope cannot be created explicitly.

It also address proper test cases for all the above.

(cherry picked from commit 0eabbac)
leandron pushed a commit to leandron/tvm that referenced this pull request Feb 2, 2023
@cbalint13 @leandron
[BugFix][UMA] Protect target registration (apache#13624)
2f999d6 
This PR address fixes for UMA target registration.
* Fix the doc issue apache#13304 
* Continues stalled PR apache#12731 

Changes:
* Incorporates all proposed fixes from mentioned [PR apache#12731](apache#12731)
* Address test case concerns and discussions from [PR apache#12731](apache#12731)
* **NEW:** Already exiting target cannot be created, explicit error on this.
* **NEW:** Attributes having special/reserved scope cannot be created explicitly.

It also address proper test cases for all the above.
tqchen pushed a commit to tqchen/tvm that referenced this pull request Feb 20, 2023
@cbalint13 @tqchen
[BugFix][UMA] Protect target registration (apache#13624)
e0ebf28 
This PR address fixes for UMA target registration.
* Fix the doc issue apache#13304 
* Continues stalled PR apache#12731 

Changes:
* Incorporates all proposed fixes from mentioned [PR apache#12731](apache#12731)
* Address test case concerns and discussions from [PR apache#12731](apache#12731)
* **NEW:** Already exiting target cannot be created, explicit error on this.
* **NEW:** Attributes having special/reserved scope cannot be created explicitly.

It also address proper test cases for all the above.
@tqchen tqchen mentioned this pull request Feb 20, 2023
Merged
tqchen pushed a commit to tqchen/tvm that referenced this pull request Feb 20, 2023
@cbalint13 @tqchen
[BugFix][UMA] Protect target registration (apache#13624)
50b33ac 
This PR address fixes for UMA target registration.
* Fix the doc issue apache#13304
* Continues stalled PR apache#12731

Changes:
* Incorporates all proposed fixes from mentioned [PR apache#12731](apache#12731)
* Address test case concerns and discussions from [PR apache#12731](apache#12731)
* **NEW:** Already exiting target cannot be created, explicit error on this.
* **NEW:** Attributes having special/reserved scope cannot be created explicitly.

It also address proper test cases for all the above.

Signed-off-by: tqchen <[email protected]>
tqchen pushed a commit to tqchen/tvm that referenced this pull request Feb 20, 2023
@cbalint13 @tqchen
[BugFix][UMA] Protect target registration (apache#13624)
8141744 
This PR address fixes for UMA target registration.
* Fix the doc issue apache#13304
* Continues stalled PR apache#12731

Changes:
* Incorporates all proposed fixes from mentioned [PR apache#12731](apache#12731)
* Address test case concerns and discussions from [PR apache#12731](apache#12731)
* **NEW:** Already exiting target cannot be created, explicit error on this.
* **NEW:** Attributes having special/reserved scope cannot be created explicitly.

It also address proper test cases for all the above.

Signed-off-by: tqchen <[email protected]>
tqchen added a commit to tqchen/tvm that referenced this pull request Feb 20, 2023
@tqchen @cbalint13
[BugFix][UMA] Protect target registration (apache#13624)
a2c01ac 
This PR address fixes for UMA target registration.
* Fix the doc issue apache#13304
* Continues stalled PR apache#12731

Changes:
* Incorporates all proposed fixes from mentioned [PR apache#12731](apache#12731)
* Address test case concerns and discussions from [PR apache#12731](apache#12731)
* **NEW:** Already exiting target cannot be created, explicit error on this.
* **NEW:** Attributes having special/reserved scope cannot be created explicitly.

It also address proper test cases for all the above.

Co-authored-by: Balint Cristian <[email protected]>
@tqchen tqchen mentioned this pull request Feb 20, 2023
Merged
tqchen added a commit to tqchen/tvm that referenced this pull request Feb 20, 2023
@tqchen @cbalint13
[BugFix][UMA] Protect target registration (apache#13624) (#2)
7002557 
This PR address fixes for UMA target registration.
* Fix the doc issue apache#13304
* Continues stalled PR apache#12731

Changes:
* Incorporates all proposed fixes from mentioned [PR apache#12731](apache#12731)
* Address test case concerns and discussions from [PR apache#12731](apache#12731)
* **NEW:** Already exiting target cannot be created, explicit error on this.
* **NEW:** Attributes having special/reserved scope cannot be created explicitly.

It also address proper test cases for all the above.

Signed-off-by: tqchen <[email protected]>
Co-authored-by: Balint Cristian <[email protected]>
@tqchen tqchen mentioned this pull request Feb 20, 2023
Merged
tqchen added a commit to tqchen/tvm that referenced this pull request Feb 20, 2023
@tqchen @cbalint13
[BugFix][UMA] Protect target registration (apache#13624) (#3)
e5906e1 
This PR address fixes for UMA target registration.
* Fix the doc issue apache#13304
* Continues stalled PR apache#12731

Changes:
* Incorporates all proposed fixes from mentioned [PR apache#12731](apache#12731)
* Address test case concerns and discussions from [PR apache#12731](apache#12731)
* **NEW:** Already exiting target cannot be created, explicit error on this.
* **NEW:** Attributes having special/reserved scope cannot be created explicitly.

It also address proper test cases for all the above.

Signed-off-by: tqchen <[email protected]>
Co-authored-by: Balint Cristian <[email protected]>
@tqchen tqchen mentioned this pull request Feb 20, 2023
Merged
tqchen added a commit to tqchen/tvm that referenced this pull request Feb 20, 2023
@tqchen @cbalint13
[BugFix][UMA] Protect target registration (apache#13624) (#4)
8c1361e 
This PR address fixes for UMA target registration.
* Fix the doc issue apache#13304
* Continues stalled PR apache#12731

Changes:
* Incorporates all proposed fixes from mentioned [PR apache#12731](apache#12731)
* Address test case concerns and discussions from [PR apache#12731](apache#12731)
* **NEW:** Already exiting target cannot be created, explicit error on this.
* **NEW:** Attributes having special/reserved scope cannot be created explicitly.

It also address proper test cases for all the above.

Signed-off-by: tqchen <[email protected]>
Co-authored-by: Balint Cristian <[email protected]>
tqchen added a commit that referenced this pull request Feb 20, 2023
@tqchen @cbalint13
[BugFix][UMA] Protect target registration (#13624) (#14057)
9bc6532 
[BugFix][UMA] Protect target registration (#13624) (#4)

This PR address fixes for UMA target registration.
* Fix the doc issue #13304
* Continues stalled PR #12731

Changes:
* Incorporates all proposed fixes from mentioned [PR #12731](#12731)
* Address test case concerns and discussions from [PR #12731](#12731)
* **NEW:** Already exiting target cannot be created, explicit error on this.
* **NEW:** Attributes having special/reserved scope cannot be created explicitly.

It also address proper test cases for all the above.

Signed-off-by: tqchen <[email protected]>
Co-authored-by: Balint Cristian <[email protected]>
fzi-peccia pushed a commit to fzi-peccia/tvm that referenced this pull request Mar 27, 2023
@cbalint13 @fzi-peccia
[BugFix][UMA] Protect target registration (apache#13624)
54358e0 
This PR address fixes for UMA target registration.
* Fix the doc issue apache#13304 
* Continues stalled PR apache#12731 

Changes:
* Incorporates all proposed fixes from mentioned [PR apache#12731](apache#12731)
* Address test case concerns and discussions from [PR apache#12731](apache#12731)
* **NEW:** Already exiting target cannot be created, explicit error on this.
* **NEW:** Attributes having special/reserved scope cannot be created explicitly.

It also address proper test cases for all the above.
mikeseven pushed a commit to mikeseven/tvm that referenced this pull request Sep 27, 2023
@cbalint13
[BugFix][UMA] Protect target registration (apache#13624)
6e56bb4 
This PR address fixes for UMA target registration.
* Fix the doc issue apache#13304 
* Continues stalled PR apache#12731 

Changes:
* Incorporates all proposed fixes from mentioned [PR apache#12731](apache#12731)
* Address test case concerns and discussions from [PR apache#12731](apache#12731)
* **NEW:** Already exiting target cannot be created, explicit error on this.
* **NEW:** Attributes having special/reserved scope cannot be created explicitly.

It also address proper test cases for all the above.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@areusch areusch areusch left review comments

@lhutton1 lhutton1 lhutton1 left review comments

@manupak manupak manupak left review comments

@cbalint13 cbalint13 cbalint13 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.
5 participants
@MichaelJKlaiber @areusch @cbalint13 @lhutton1 @manupak