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

[RFC][TVMScript] New namespace for tvmscript #36

Merged
merged 2 commits into from Sep 29, 2021
Merged

Conversation

Hzfengsy
Copy link
Member

@Hzfengsy Hzfengsy commented Sep 23, 2021

It's a rfc for changing TVMScript namespace to enable auto-completion support and pass pylint checks.

# ^ there is a broadly accepted precedence in doing this in the python community:
# from keras import backend as K

@tvm.script.ir_module # it generates an IRModule
Copy link
Member

Choose a reason for hiding this comment

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

should it be IRModule directly or ir_module?

Copy link
Member Author

@Hzfengsy Hzfengsy Sep 24, 2021

Choose a reason for hiding this comment

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

Personally prefer ir_module, because decorators are python functions, which usually use snake_case style. But other suggestions from the community are welcomed

Copy link
Contributor

@tkonolige tkonolige left a comment

Choose a reason for hiding this comment

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

I really like this change. Can't wait for autocomplete!


@tvm.script.ir_module # it generates an IRModule
class Module:
@T.prim_func # it generates a PrimFunc explicitly
Copy link
Contributor

Choose a reason for hiding this comment

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

Current we do not require decorators for functions within a class decorated with tvm.script.tir. Are you suggesting we change this so all functions need a decorator?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. I think it is needed in the long term (since IRModule contains not only PrimFunc but also functions for relay). However, only PrimFuncs are supported by TVMScript by now. It's OK not to decorate functions in this PFC.

Love to hear your opinions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree we need it in the future. I don't really care if we add it now or later, we should just make the decision explicit if we do add it.


[drawbacks]: #drawbacks

Here are some existing works based on current TVM Script syntax. It need a huge refactor to migrate it to the new one.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way we could deprecate but not remove the old syntax. It might make it easier to transition. (Though it might be impossible because tir is changing from a function to a module. Stackoverflow suggests some very hacky solutions).

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. Actually I have refactored all testcase on my local machine. Will pr very soon. The only problem is how to review such huge refactoring.

Copy link
Contributor

@MasterJH5574 MasterJH5574 left a comment

Choose a reason for hiding this comment

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

I noticed a minor point. @Hzfengsy Please take a look! Thanks!

rfcs/0036-tvm-script-namespace.md Show resolved Hide resolved
rfcs/0036-tvm-script-namespace.md Show resolved Hide resolved
@altanh
Copy link

altanh commented Sep 27, 2021

I support this change. One suggestion is that we keep a canonical namespace that will always be parsed correctly to the TIR namespace (e.g. tir.block will always work), and in particular use this identifier during printing. Mentioning this here since I noticed the PR apache/tvm#9115 is printing using the T alias, which seems problematic unless we print an import statement at the top.

@Hzfengsy
Copy link
Member Author

Thanks, @altanh. Your suggestion makes sense to me. To be specific, here are two cases: parse from a python script and string.

  1. When we parse from a python script, we detect the prefix T from the python env (through function __globals__, i.e. you can even use XXX.block if with from tvm.script import tir as XXX)
  2. When we parse from a text string (e.g. printed from tir), we can specify the accept prefixes by from_source(source_code, tir_prefix=["T", "tir"]) (default accept both tir and T). Also, we can choose the prefix during printing by func.script(tir_prefix=“tir”) (default tir)

I would update the PR with the new proposal soon. Please feel free to left your comments about the changes.

@tqchen
Copy link
Member

tqchen commented Sep 28, 2021

Great discussions. If we are encouraging a common idiom(e.g. use T for tir).

We might actually want to encourage print in T as prefix to be consistent on what users might write(and further edit with auto-completion), assuming we always encourage user write from tvm.script import tir as T. As long as we agree to that convention. Having a prefix option indeed makes the choice flexible.

@junrushao
Copy link
Member

Thanks guys for the discussion! If the comments and concerns are all addressed, let's move on and merge this PR after 24h

@junrushao junrushao merged commit 4695608 into apache:main Sep 29, 2021
@junrushao
Copy link
Member

Thanks guys for the discussion! The RFC is merged :-)

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.

None yet

6 participants