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

[TVMSCRIPT] Using diagnostics for TVM Script #6797

Merged
merged 6 commits into from
Nov 6, 2020
Merged

[TVMSCRIPT] Using diagnostics for TVM Script #6797

merged 6 commits into from
Nov 6, 2020

Conversation

tkonolige
Copy link
Contributor

This PR modifies TVM Script to use the new diagnostics interface. This should make error messages a lot clearer.

This PR depends on #6795.

@jroesch @tqchen @junrushao1994

@tqchen
Copy link
Member

tqchen commented Oct 30, 2020

cc @spectrometerHBH

@tqchen
Copy link
Member

tqchen commented Oct 30, 2020 •
edited
Loading

Please wait until the jenkinsfile get updated, we will need a separate PR to update the Jenkinsfile first after ensuring the docker staging pass (to prevent other test regressions), will send an update once the image get updated.

@tkonolige tkonolige force-pushed the tvmscript_diagnostics_reredux branch from f84943d to db004ab Compare October 30, 2020 16:55
u99127
u99127 reviewed Oct 30, 2020
View reviewed changes
python/tvm/script/diagnostics.py Outdated
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
"""Bridge from synr DiagnosticContext to TVM's diagnostics"""
Copy link
Contributor

Choose a reason for hiding this comment

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

What is synr ? I don't obviously follow what it means - a quick google search doesn't indicate anything either.

regards
Ramana

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Synr is a small utility library we wrote to normalize the python ast between version changes. You can find it here: https://github.com/octoml/synr.

I'll reword this comment to make it clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

The comment is not very clear as it only refers to linking a Diagnostic context in synr to tvm. Is there an RFC or something that relates to this that you could point people at who might be interested in what problem this is solving and why this is needed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you parse the AST with synr, you also pass in a object implementing synr's DiagnosticContext interface (https://synr.readthedocs.io/en/latest/#synr.DiagnosticContext). The DiagnosticContext is responsible for accumulating and reporting errors. This file defines a wrapper object that implements synr's DiagnosticContext and calls out to TVMCtx to report errors.

I think the closest RFC you will find is https://discuss.tvm.apache.org/t/rfc-meta-rfc-3-pronged-plan-for-improving-error-messages-in-tvm/7214.

@tqchen
Copy link
Member

tqchen commented Nov 2, 2020

docker images has been updated

@tkonolige tkonolige force-pushed the tvmscript_diagnostics_reredux branch from db004ab to e2b7319 Compare November 2, 2020 18:27
@junrushao
Copy link
Member

@spectrometerHBH Would you like to take a look? Thanks!

@spectrometerHBH
Copy link
Contributor

Is this PR ready for review? Looks like it encountered problems in CI.

@tkonolige
Copy link
Contributor Author

@spectrometerHBH Feel free to review it now, I need to bump the dependency in docker.

spectrometerHBH
spectrometerHBH approved these changes Nov 3, 2020
View reviewed changes
Copy link
Contributor

@spectrometerHBH spectrometerHBH left a comment

Choose a reason for hiding this comment

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

LGTM!

python/tvm/script/meta_unparser.py


class MetaUnparser(ast.NodeVisitor):
class MetaUnparser(Transformer):
Copy link
Contributor

Choose a reason for hiding this comment

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

We can consider renaming this class.

leandron
leandron reviewed Nov 4, 2020
View reviewed changes
python/tvm/script/parser.py Outdated Show resolved Hide resolved
python/tvm/script/parser.py Show resolved Hide resolved
python/tvm/script/parser.py Outdated Show resolved Hide resolved
python/tvm/script/parser.py Outdated Show resolved Hide resolved
python/tvm/script/parser.py Outdated Show resolved Hide resolved
python/tvm/script/parser.py Outdated Show resolved Hide resolved
@tkonolige tkonolige force-pushed the tvmscript_diagnostics_reredux branch from 0a4a61b to c7518ae Compare November 4, 2020 18:05
@junrushao
Copy link
Member

Do we prefer to maintain synr as an external dependency, and make a release every time, or include synr as part of tvm?

@jroesch
Copy link
Member

jroesch commented Nov 4, 2020

Do we prefer to maintain synr as an external dependency, and make a release every time, or include synr as part of tvm?

I think it is valuable to maintain outside of the TVM code base. I know monorepo'ing everything provides some conveniences but when there are reusable components which can be used outside of TVM it makes sense to maintain them as separate pieces of code. For example there have been multiple other projects which have built similar approaches to synr and I think it is valuable to provide it in an easily reusable component, for example TensorFlow and PyTorch have both opt'd not to do this and even though they do AST rewriting there is no easy way to take advantage of all the work they did.

@tkonolige tkonolige force-pushed the tvmscript_diagnostics_reredux branch from 5fa9aa3 to fd54e42 Compare November 5, 2020 17:59
@tkonolige
Copy link
Contributor Author

@leandron Does this PR look good to you now? Or are there any other changes you would like?

@tqchen tqchen merged commit d164aac into apache:main Nov 6, 2020
@tqchen
Copy link
Member

tqchen commented Nov 6, 2020

Thansk @tkonolige @leandron @u99127 @jroesch @junrushao1994 @spectrometerHBH !

@tkonolige tkonolige mentioned this pull request Nov 6, 2020
Merged
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Dec 2, 2020
@tkonolige @leandron
[TVMSCRIPT] Using diagnostics for TVM Script (apache#6797)
9d30c63 
* [TVMSCRIPT] Using diagnostics for TVM Script

* fix lint

* More documentation, improve some error messages

* Apply suggestions from code review

Co-authored-by: Leandro Nunes <[email protected]>

* Add synr to ci setup and setup.py

* remove typed_ast dependency

Co-authored-by: Leandro Nunes <[email protected]>
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Dec 4, 2020
@tkonolige @leandron
[TVMSCRIPT] Using diagnostics for TVM Script (apache#6797)
b1aa82c 
* [TVMSCRIPT] Using diagnostics for TVM Script

* fix lint

* More documentation, improve some error messages

* Apply suggestions from code review

Co-authored-by: Leandro Nunes <[email protected]>

* Add synr to ci setup and setup.py

* remove typed_ast dependency

Co-authored-by: Leandro Nunes <[email protected]>
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Dec 4, 2020
@tkonolige @leandron @trevor-m
[TVMSCRIPT] Using diagnostics for TVM Script (apache#6797)
d19736d 
* [TVMSCRIPT] Using diagnostics for TVM Script

* fix lint

* More documentation, improve some error messages

* Apply suggestions from code review

Co-authored-by: Leandro Nunes <[email protected]>

* Add synr to ci setup and setup.py

* remove typed_ast dependency

Co-authored-by: Leandro Nunes <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@u99127 u99127 u99127 left review comments

@leandron leandron leandron left review comments

@spectrometerHBH spectrometerHBH spectrometerHBH approved these changes

@tqchen tqchen tqchen 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.
7 participants
@tkonolige @tqchen @junrushao @spectrometerHBH @jroesch @leandron @u99127