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

[MetaSchedule][M3b] Database #9061

Merged
merged 2 commits into from
Sep 28, 2021
Merged

[MetaSchedule][M3b] Database #9061

merged 2 commits into from
Sep 28, 2021

Conversation

junrushao
Copy link
Member

@junrushao junrushao commented Sep 21, 2021

This PR is part of the meta schedule project (#8473) that adds a workload registry, as well as
a generic database of tuning records. This feature is future-compatible with dynamic shape
auto-tuning work by @ZihengJiang and @ArmageddonKnight.

Depend on PR #9059. The PR is ready for review now!

Co-authored-by: Xiyou Zhou <[email protected]>
Co-authored-by: Bohan Hou <[email protected]>
Co-authored-by: Ruihang Lai <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Wuwei Lin <[email protected]>
Co-authored-by: Siyuan Feng <[email protected]>

@junrushao
Copy link
Member Author

@comaniac
Copy link
Contributor

Will try to find time reviewing this PR this afternoon or tomorrow.

@comaniac
Copy link
Contributor

Took a fast pass and had an offline discussion with @junrushao1994. Two major points:

  1. The APIs are bit confusing (e.g.,lookup_or_add and add). It might be better to use something like commit_workload and commit_record. Meanwhile, since we may add more database implementations in the future (e.g., sqlite), this general API can also make sure we won't need to refine it again in AutoTIR.
  2. Better to remove workload registry accordingly to make the design simple.

@junrushao
Copy link
Member Author

Per discussion with @comaniac.

The naming change:

  • lookup_or_add seems pretty random, and we should rename it as commit_workload which is more consistent with database terminologies
  • add seems okay but commit_tuning_record sounds better and more consistent with commit_workload

Minor change in the code structure:

  • Consolidate WorkloadRegistry into JSONFile database, so that there is only one unified database API, and this could mimic the two-table database behavior

Copy link
Contributor

@mbrookhart mbrookhart left a comment

Choose a reason for hiding this comment

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

First kind of stream of consciousness pass through the code. Mostly documentation/comment requests. It seems the direct use of the Database is all wrapped in a JSON-related file object, which kind of confuses me, would you mind providing a brief explanation of the class structure somewhere?

include/tvm/meta_schedule/database.h Outdated Show resolved Hide resolved
include/tvm/meta_schedule/database.h Outdated Show resolved Hide resolved
include/tvm/meta_schedule/workload_registry.h Outdated Show resolved Hide resolved
include/tvm/meta_schedule/workload_registry.h Outdated Show resolved Hide resolved
include/tvm/meta_schedule/workload_registry.h Outdated Show resolved Hide resolved
python/tvm/meta_schedule/database/database.py Outdated Show resolved Hide resolved
python/tvm/meta_schedule/workload_registry.py Outdated Show resolved Hide resolved
src/meta_schedule/database/json_file.cc Outdated Show resolved Hide resolved
src/meta_schedule/database/json_file.cc Outdated Show resolved Hide resolved
python/tvm/meta_schedule/database/json_file.py Outdated Show resolved Hide resolved
@junrushao
Copy link
Member Author

junrushao commented Sep 23, 2021

It seems the direct use of the Database is all wrapped in a JSON-related file object, which kind of confuses me, would you mind providing a brief explanation of the class structure somewhere?

Hey thanks @mbrookhart for asking! Let's focus on the database API and ignore the workload registry part (which will be removed according to previous discussion with Cody in this thread).

  • The base abstract class is Database, which has 4 pure virtual methods: commit_workload, commit_tuning_record, get_top_k, size
  • The system interacts with any subclass of Database by calling these 4 methods
  • To implement a concrete subclass of Database: in pure C++, we can just inherit Database and do so; in pure python, we provide PyDatabase to inherit from, where we plays some tricks, so that everything can be implemented in pure python
  • JSONFile is a simple default implementation of Database for open source
  • Developers can provide other implementations too by inheriting Database or PyDatabase, for example, database backed by SQLite, etc, as long as the class implements the 4 methods

@mbrookhart
Copy link
Contributor

The first three points and the last one make sense, but I think I'm a little confused by JSONFile being the default user-facing database. JSON means a very particular thing, you convert to/from it, but it doesn't have class methods or a user interface, per say. I would say all of the JSON functionality is important for conversion and serialization, but I'm not sure it makes sense for it to be the primary interface point?

@junrushao
Copy link
Member Author

@mbrookhart Yeah indeed JSONFile can be very confusing given JSON means very particular thing. Would love to hear about your proposals

@junrushao
Copy link
Member Author

@mbrookhart @comaniac I updated the patch according to your comments. Major changes:

  • Remove WorkloadRegistry, WorkloadToken and the annoying token_id_
  • Rename lookup_or_add => commit_workload
  • Rename add => commit_tuning_record
  • Various refactors to consolidate more stuff

Let me know what you guys think :-)

@junrushao
Copy link
Member Author

BTW, here is the convention we are using in TensorIR and meta schedule project:

  • An abstract base class that describe how the interface looks like, e.g. Schedule, Database, Builder, Runner
  • Concrete implementation as subclasses are hidden in src/ folder, and mostly hidden in cc files, which are not directly visible to developers. For example, PyDatabase, ConcreteSchedule
  • The abstract class provides factory methods to create these concrete implementations, whose name is consistent with the names of the subclasses. For example:
class Database : ... {
 public:
  static Database JSONFile(...); // TBD: Rename 
  static Database SQLiteDB(...);
};

And then another problem here is what name should we change JSONFile for

Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

The refactored version is much cleaner :D
Left some comments mostly regarding to the naming. Otherwise LGTM.

include/tvm/meta_schedule/database.h Outdated Show resolved Hide resolved
python/tvm/meta_schedule/database/database.py Outdated Show resolved Hide resolved
python/tvm/meta_schedule/database/database.py Outdated Show resolved Hide resolved
python/tvm/meta_schedule/database/json_file.py Outdated Show resolved Hide resolved
tests/python/unittest/test_meta_schedule_database.py Outdated Show resolved Hide resolved
@junrushao junrushao changed the title [Meta Schedule][M3c] Database [Meta Schedule][M3b] Database Sep 24, 2021
This PR is part of the meta schedule project (#8473) that adds a generic
Database interface of tuning records, as well as a default implementation
of using two JSON-files to mimic the database.
This feature is future-compatible with dynamic shape auto-tuning.

Co-authored-by: Xiyou Zhou <[email protected]>
Co-authored-by: Bohan Hou <[email protected]>
Co-authored-by: Ruihang Lai <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Wuwei Lin <[email protected]>
Co-authored-by: Siyuan Feng <[email protected]>
Copy link
Contributor

@mbrookhart mbrookhart left a comment

Choose a reason for hiding this comment

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

I think I agree with @comaniac here, definitely much cleaner now, and I think the only thing I'm still concerned about is naming. Maybe we could just call it JSONDatabase? I think that's a little less ambiguous but still succinct.

Otherwise, LGTM

include/tvm/meta_schedule/database.h Show resolved Hide resolved
@junrushao
Copy link
Member Author

Hey thanks guys for the discussion! Let's go with the name JSONDatabase then 😄

@junrushao
Copy link
Member Author

PR updated according to our discussion 👍 @mbrookhart @comaniac please take another look. Thanks guys for the discussion!

Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

Copy link
Contributor

@mbrookhart mbrookhart left a comment

Choose a reason for hiding this comment

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

Thanks, Junru!

@junrushao junrushao merged commit 9e47b43 into apache:main Sep 28, 2021
@junrushao
Copy link
Member Author

Thanks everybody for the review :-)

AndrewZhaoLuo added a commit to AndrewZhaoLuo/tvm that referenced this pull request Sep 28, 2021
* main: (37 commits)
  [ONNX] [Relay] Dynamic squeeze (apache#9095)
  [Meta Schedule][M3b] Database (apache#9061)
  [Bugfix] Add nullptr checking for `AttrStmt` with `coproc_uop_scope` attr key (apache#9123)
  [Codegen] Swap out analyzer when outlining (apache#9117)
  [CI] bash.sh, build.sh: add option to set the container name and hostname (apache#9110)
  Ensure google-mock is installed and setup (apache#9107)
  Arm(R) Ethos(TM)-U NPU TIR to CS for Conv2D (apache#8811)
  Frontend: add onnx GlobalLpPool op (apache#8845)
  [LLVM] Refactor MakeCallPacked, NFC (apache#9118)
  prevent casting handle to other types (apache#9114)
  fix annotation of tir generic (apache#9119)
  [Relay] Register layout conversion function to more reduce ops (apache#9048)
  Fix the missing `dtype` attribute of `tir.Shuffle` in Python level (apache#9131)
  add `multiply` and remove `subtract` for dnnl json runtime (apache#9120)
  relu of dnnl json runtime only support 4-dims input (apache#9122)
  [Meta Schedule][M3a] SpaceGenerator  (apache#9079)
  [TensorIR][Bugfix] Disallow fusing loops with dependency (apache#9112)
  adding Jorn to reviewers list (apache#9105)
  [BYOC] Fix incorrect conv2d padding handling of `dnnl with c source runtime` (apache#9097)
  [Frontend][TFLite] fix apache#9078 (apache#9099)
  ...
ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
This PR is part of the meta schedule project (apache#8473) that adds a generic
Database interface of tuning records, as well as a default implementation
of using two JSON-files to mimic the database.
This feature is future-compatible with dynamic shape auto-tuning.

Co-authored-by: Xiyou Zhou <[email protected]>
Co-authored-by: Bohan Hou <[email protected]>
Co-authored-by: Ruihang Lai <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Wuwei Lin <[email protected]>
Co-authored-by: Siyuan Feng <[email protected]>
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
This PR is part of the meta schedule project (apache#8473) that adds a generic
Database interface of tuning records, as well as a default implementation
of using two JSON-files to mimic the database.
This feature is future-compatible with dynamic shape auto-tuning.

Co-authored-by: Xiyou Zhou <[email protected]>
Co-authored-by: Bohan Hou <[email protected]>
Co-authored-by: Ruihang Lai <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Wuwei Lin <[email protected]>
Co-authored-by: Siyuan Feng <[email protected]>
@junrushao junrushao changed the title [Meta Schedule][M3b] Database [MetaSchedule][M3b] Database Jan 26, 2022
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.

4 participants