[Relay][Module] Refactor the way we interface between different modules of Relay. #3906
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
weberlo
suggested changes
Sep 6, 2019
jroesch
force-pushed
the
module-refactor
branch
from
129389a to
95d1a0f
Compare
zhiics
approved these changes
Sep 10, 2019
There was a problem hiding this comment.
Great to see this PR. Reuse of prelude gave me errors before as well. Please fix some style issues pointed out by @MarisaKirisame and @weberlo. I will merge once CI passes.
weberlo
approved these changes
Sep 11, 2019
|
Thanks everyone, this is now merged. |
tqchen
reviewed
Sep 12, 2019
| * \brief Import Relay code from the file at path, relative to the standard library. | ||
| * \param path The path of the Relay code to import. | ||
| */ | ||
| TVM_DLL void ImportFromStd(const std::string& path); |
There was a problem hiding this comment.
From users' perspective, Import and ImportFromStd seems to be a bit too similar and introduces metal overhead about choosing between them, can be combine them into one?
| * if abosolute will be the absolute file, if | ||
| * relative it will be resovled against the current | ||
| * working directory. | ||
| */ |
There was a problem hiding this comment.
The current API mutates the module, this is a bit diverging from designs of trying to be immutable as much as possible. How about relay.Module.fromtext and relay.Module.load?
wweic
pushed a commit
to wweic/tvm
that referenced
this pull request
Sep 16, 2019
…es of Relay. (apache#3906) * Module refactor * Add load module * Add support for idempotent import * Tweak load paths * Move path around * Expose C++ import functions in Python * Fix import * Add doc string * Fix * Fix lint * Fix lint * Fix test failure * Add type solver * Fix lint
wweic
pushed a commit
to wweic/tvm
that referenced
this pull request
Sep 16, 2019
…es of Relay. (apache#3906) * Module refactor * Add load module * Add support for idempotent import * Tweak load paths * Move path around * Expose C++ import functions in Python * Fix import * Add doc string * Fix * Fix lint * Fix lint * Fix test failure * Add type solver * Fix lint
wweic
pushed a commit
to neo-ai/tvm
that referenced
this pull request
Sep 16, 2019
…es of Relay. (apache#3906) * Module refactor * Add load module * Add support for idempotent import * Tweak load paths * Move path around * Expose C++ import functions in Python * Fix import * Add doc string * Fix * Fix lint * Fix lint * Fix test failure * Add type solver * Fix lint
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduces an idempotent import mechanism for Relay and restructures some of the code around handling Relay files. One of the challenges for introducing new dialects of Relay as I was doing in #3560 is the ability to define new external types, and operations over them.
In order to track new types such as
Storagewe need the ability to share chunks of code in a idempotent way. The current module design is lacking in this regard and the import mechanism addresses this.An example of where this fails now is if we introduce new builtin types, where do we place the code to install them in a module? for example if we use the instantiate the prelude more than once we will receive an error right now.
This is required for my memory analysis PR which is upcoming, and depends on @weberlo's parser PR.
@weberlo will follow up by moving the entire Prelude to the text format.
This PR also cleans up InferType's handling of modules which is currently sub-optimal.