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

[Refactor][Relay] Refactor Relay Python to use new FFI #5077

Merged
merged 4 commits into from
Mar 17, 2020

Conversation

zhiics
Copy link
Member

@zhiics zhiics commented Mar 16, 2020

#5068

This PRs refactors Relay to

  1. Reflect the C++ side organization to keep consistency
  2. Separated analysis and transform namespace to make the namespace for different optimizations clear
  3. Introduced the new _ffi_api for each namespace and remove the old _xx.py files
  4. Removed some op related files to import modules from subdirectories and instead import then from init.py directly to keep Relay directory clean

Three followup PRs are needed to:

  1. Create a function.py file for relay.Function
  2. Move the files that are left over to the backend directory
  3. Docs

cc @tqchen @jroesch @icemelon9 @slyubomirsky @MarisaKirisame @wweic @yzhliu

python/tvm/relay/__init__.py Outdated Show resolved Hide resolved
@tqchen
Copy link
Member

tqchen commented Mar 16, 2020

Some comments about docs and structure exposure.

@tqchen
Copy link
Member

tqchen commented Mar 16, 2020

In the case of the file groupings for relay/expr.py etc. There are two ways to think about it. We can either make them consistent with src/, which means we want relay/ir/expr.py, or first to be consistent with the include, in which case it is relay/expr.py.

Given that tir already tries to be consistent with include first(as it is user facing), let us try to also do that in relay

@zhiics zhiics force-pushed the relay_py_refactor branch from 6b0031d to bc65305 Compare March 16, 2020 18:21
@zhiics
Copy link
Member Author

zhiics commented Mar 16, 2020

@tqchen I updated the change. Moving the files back to the relay folder actually also resolves the user facing python API concern.

@tqchen
Copy link
Member

tqchen commented Mar 16, 2020

Thanks @zhiics please also take note about the above comments for docs in general(see the new usage of automodule that removes the need of manual autofunction), but we can do that as a followup if CI is green

Copy link
Member

@tqchen tqchen left a comment

Choose a reason for hiding this comment

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

some further comments

python/tvm/relay/__init__.py Outdated Show resolved Hide resolved
python/tvm/relay/__init__.py Outdated Show resolved Hide resolved
python/tvm/relay/__init__.py Outdated Show resolved Hide resolved
@tqchen tqchen added the status: need update need update based on feedbacks label Mar 17, 2020
@zhiics zhiics force-pushed the relay_py_refactor branch from 387c1db to a5cf36b Compare March 17, 2020 02:28
@zhiics
Copy link
Member Author

zhiics commented Mar 17, 2020

@tqchen PTAL, thanks.

@tqchen tqchen merged commit 4aff8dc into apache:master Mar 17, 2020
@tqchen
Copy link
Member

tqchen commented Mar 17, 2020

Thanks @zhiics

@tqchen tqchen added status: accepted and removed status: need review status: need update need update based on feedbacks labels Mar 17, 2020
@zhiics zhiics deleted the relay_py_refactor branch March 17, 2020 17:32
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Apr 16, 2020
* refactor relay python

* revert relay/ir/*.py to relay

* Address comments

* remove direct access to analysis and transform namespace
zhiics added a commit to neo-ai/tvm that referenced this pull request Apr 17, 2020
* refactor relay python

* revert relay/ir/*.py to relay

* Address comments

* remove direct access to analysis and transform namespace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants