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

[DOCS] Add docs for Pass Instrument #8220

Merged
merged 20 commits into from
Jul 7, 2021
Merged

[DOCS] Add docs for Pass Instrument #8220

merged 20 commits into from
Jul 7, 2021

Conversation

chiwwang
Copy link
Contributor

@chiwwang chiwwang commented Jun 9, 2021

Hi,
This is a follow-up PR on #7952
A How-To-Use-Pass-Instrument tutorial is added.
Pass Instrument sections are also added to pass_infra.rst.

[DOCS] Add docs for Pass Instrument

  • Add a tutorial about how to use pass instrument.
  • Add related sections in Pass Infrastructure documents.

@zackcquic @tqchen @areusch @tqchen @tkonolige @altanh

Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.

chiwwang and others added 9 commits May 14, 2021 22:34
Initiate a Path object from TEST_DATA_ROOT_PATH to fix the error:
AttributeError: 'str' object has no attribute 'mkdir'
 - Add a tutorial about how to use pass instrument.
 - Add related sections in Pass Infrastructure documents.
Copy link
Contributor

@zackcquic zackcquic left a comment

Choose a reason for hiding this comment

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

Hi @chiwwang:

Thanks a lot :)

Something should be added:

  1. What happens when exceptions occur in different instrument point.
  2. Standard Instrument section: PassTimingInstrument, PrintBefore(TODO), PrintAfter(TODO), ..
  3. Global PassContext and override_instrument examples
  4. use_pass_infra.py's comments should be updated, sorry, I forgot to update it.
  5. conf.py should be updated.

docs/api/python/ir.rst Outdated Show resolved Hide resolved
``PassInstrument`` provides callbacks run by these methods. Multiple
``PassInstrument`` instances can be registed into a single ``PassContext``.
They are called sequentially in the order of ``instruments`` member.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add exception situations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. But it's a little long section. Please help review. Thanks:)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it maybe better to move python frontend ahead, use it to introduce what is a pass instrument and instrument points, then goes to the discussion in PassContext::Instrument*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's hard to introduce python frontend first because pass_infra.txt put a big section called "Python Frontend" downward.
All python things are put there. If I don't follow this, pass_instra.txt become fragmented.
(Although I agreed it is easier to introduce call-sequences and concepts with Python code...)

docs/dev/pass_infra.rst Outdated Show resolved Hide resolved
As more and more passes are implemented, it becomes interesting to instrument
passes execution, analyze per-pass effects and observe various events.
We have extended :py:class:`tvm.transform.PassContext` to accept a list of
instrument classes. Also a decorator :py:func:`tvm.ir.instrument.pass_instrument` is provided to easily implement instrument classes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Pass instrument framework extended ... instrument instances, Also a decorator :py:func:tvm.instrument.pass_instrument ... instrument instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think pass_instrument is used to help implement a "class", not a instance.
And argument "instruments" accept a list of instances. Do I understand correctly?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, you are right.

So it should be,
Pass instrument framework extended ... instrument instances, Also a decorator :py:func:tvm.instrument.pass_instrument ... instrument classes.
Sorry for miss-copied the second one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't quite get what "..." should be in "Pass instrument framework extended ... instrument instances".
But wordings around here is changed. Could you take a look again?

instrument classes. Also a decorator :py:func:`tvm.ir.instrument.pass_instrument` is provided to easily implement instrument classes.

This tutorial demostrates how developers can use ``PassContext`` to instrument
passes. For more details, please refer to the :ref:`pass-infra`
Copy link
Contributor

Choose a reason for hiding this comment

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

How about remove "For more details, ... " ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems no change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to "please also refer to ...."
Maybe you were referring to old commits?

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.

Thanks for this hard work @chiwwang! Documentation is a really important part of this project. I've left some feedback.

@@ -109,7 +109,8 @@ configure the compilation options, including optimization level and
required/disabled passes, etc. For instance, we may have a configuration which
performs all passes at ``opt_level=3`` with some disabled passes using
``disabled_pass=xx`` provided by ``PassContext``. Now we could glob all passes
at ``opt_level=3`` and exclude those in the disabled pass list.
at ``opt_level=3`` and exclude those in the disabled pass list. ``PassContext``
also provides pass-instruments mechanism, which will be introduced latter.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
also provides pass-instruments mechanism, which will be introduced latter.
also provides a way to instrument all passes. See section [fill in the section here].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -123,16 +124,23 @@ Python APIs to create a compilation pipeline using pass context.

class PassContextNode : public Object {
public:
ErrorReporter err_reporter;
Copy link
Contributor

Choose a reason for hiding this comment

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

What happened to the error reporter?

Copy link
Contributor Author

@chiwwang chiwwang Jun 11, 2021

Choose a reason for hiding this comment

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

It seems to be replaced by diag_ctx in #6274.
It is not related to this PR. I fixed it just because it's handy.
Though there aren't lots of descriptions about error reporters in pass_infra.txt, I think it should present due to this sentence:

PassContext carries useful information for an optimization pass. For example, it contains the error reporting system so optimization authors can provide diagnostic...

TVM_DLL bool InstrumentBeforePass(const IRModule& mod, const PassInfo& info) const;
TVM_DLL void InstrumentAfterPass(const IRModule& mod, const PassInfo& info) const;

The first two methods are called respectively in entering/exiting context scope. The latter two are called while passes is being applied(`src/ir/transform.cc`_).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think pseudo code of how these functions are called would be useful here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. But you might want to take a look. I am not quite confident of what syntax should be used for pseudo code.

if (pass_ctx.InstrumentBeforePass(ir_module, pass_info)) {
  new_ir_module = run_pass(ir_module, pass_ctx);
  pass_ctx.InstrumentAfterPass(new_ir_module, pass_info);
  return new_ir_module;
}

Comment on lines 414 to 415
Note that ``InstrumentBeforePass()`` return a boolean indicating this pass should
be run or not.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Note that ``InstrumentBeforePass()`` return a boolean indicating this pass should
be run or not.
Note that ``InstrumentBeforePass()`` returns a boolean indicating whether or not the pass should
be run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


``PassInstrument`` provides callbacks run by these methods. Multiple
``PassInstrument`` instances can be registed into a single ``PassContext``.
They are called sequentially in the order of ``instruments`` member.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the same as the order in which they are registered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I tried to change wordings to "in the order of instruments argument passed to PassContext"


- ``exit_pass_ctx``

* This callback is run at the moement of exiting ``PassContext``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* This callback is run at the moement of exiting ``PassContext``.
* This callback is run when exiting the ``PassContext``.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


- ``should_run``

* This callback is run before a pass is executed, returning a boolean indicating if the pass should be run.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* This callback is run before a pass is executed, returning a boolean indicating if the pass should be run.
* This callback is run before a pass is executed. It returns a boolean indicating if the pass should be run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


Examples
--------
The following code block decorates a pass instrument class.
The following code block show how to decorate a pass instrument class.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The following code block show how to decorate a pass instrument class.
The following code block shows how to decorate a pass instrument class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

==============================
**Author**: `Chi-Wei Wang <https://github.com/chiwwang>`_

As more and more passes are implemented, it becomes interesting to instrument
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
As more and more passes are implemented, it becomes interesting to instrument
As more and more passes are implemented, it becomes useful to instrument

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 45 to 59
# We create a Relay program from a Pytorch model.
# Here we pick up ``mobilenet_v2`` from torchvision.
import torch
import torchvision

model_name = "mobilenet_v2"
model = getattr(torchvision.models, model_name)(pretrained=True)
model = model.eval()

input_shape = [1, 3, 224, 224]
input_data = torch.randn(input_shape)
scripted_model = torch.jit.trace(model, input_data).eval()

shape_list = [("input0", input_shape)]
relay_mod, relay_params = relay.frontend.from_pytorch(scripted_model, shape_list)
Copy link
Contributor

Choose a reason for hiding this comment

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

Importing a pytorch model seems to unnecessarily complicate this tutorial. Why not use relay.testing.resnet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing out this API. It is not used just because I am not aware of it.
Change the example model to resnet-18.

@chiwwang
Copy link
Contributor Author

chiwwang commented Jun 11, 2021

Thanks for prompt feedbacks @zackcquic @tkonolige !

Here are some comments for Zack's questions:

  1. What happens when exceptions occur in different instrument point.
    Added in pass_infra.txt. But it is a little long. You might want to take a look again.

  2. Standard Instrument section: PassTimingInstrument, PrintBefore(TODO), PrintAfter(TODO), ..
    I think it might be better to maintain these in the doc string of related Python class/function.
    So I add example to instrument.py. It will be shown in Python API reference.

  3. Global PassContext and override_instrument examples
    Done. Sorry for not aware of this approach.

  4. use_pass_infra.py's comments should be updated, sorry, I forgot to update it.
    Done.

  5. conf.py should be updated.
    Done. But actually it seems to automatically append un-listed tutorials to the end.
    How do you think about the current order of tutorial?


Python interfaces are provided to implement ``PassInstrument`` quickly.

Following four methods are invoked in the life-cycle of ``PassContext``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently we introduces four instrument point in the life-cycle of PassContext. (introduce in python frontend maybe better)
Include the text graph in

* with PassContext(instruments=[pi]): # pi = a PassInstrument implementation
maybe helpful to understand quickly (move this to python frontend, too?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added text-graph.

``PassInstrument`` provides callbacks run by these methods. Multiple
``PassInstrument`` instances can be registed into a single ``PassContext``.
They are called sequentially in the order of ``instruments`` member.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it maybe better to move python frontend ahead, use it to introduce what is a pass instrument and instrument points, then goes to the discussion in PassContext::Instrument*

TVM_DLL void InstrumentAfterPass(const IRModule& mod, const PassInfo& info) const;

``InstrumentEnterPassContext`` is called immediately when the scope
of the ``PassContext`` instance is entered.
Copy link
Contributor

Choose a reason for hiding this comment

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

when entering the scope of PassContext instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's hard to introduce python frontend first because pass_infra.txt put a big section called "Python Frontend" downward.
All python things are put there. If I don't follow this, pass_instra.txt become fragmented.
(Although I agreed it is easier to introduce call-sequences and concepts with Python code...)

``InstrumentEnterPassContext`` is called immediately when the scope
of the ``PassContext`` instance is entered.

``InstrumentExitPassContext`` is called when the scope of ``PassContextNode``
Copy link
Contributor

Choose a reason for hiding this comment

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

when leaving the scope of PassContext instance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


- ``InstrumentBeforePass``

* ``ShouldRun`` callbakc is executed if the pass is not listed as a required pass.
Copy link
Contributor

Choose a reason for hiding this comment

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

callback

Copy link
Contributor

Choose a reason for hiding this comment

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

i think you could just delete the word callback here. ShouldRun is an interface method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

- ``InstrumentBeforePass``

* ``ShouldRun`` callbakc is executed if the pass is not listed as a required pass.
If the pass is a required pass, ``ShouldRun`` will not be executed for that pass.
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems this sentence is same with above. Maybe able to remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


``InstrumentExitPassContext`` is called when the scope of ``PassContextNode``
is being leaved, or exceptions occur during the execution of passes.
This method is also called when instruments is being overriden by ``override_instruments`` in ::py:class:`tvm.transform.PassContext`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think override_instruments should be placed in another little section to describe along with the concept of global PassContext

Copy link
Contributor Author

@chiwwang chiwwang Jun 13, 2021

Choose a reason for hiding this comment

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

Done.
Because override_instruments appears as a Python function, I put it after Python Frontend introduction in pass_infra.txt.

Pass Instrument
~~~~~~~~~~~~~~~

``PassInstrument`` provides callbacks run when entering/exiting ``PassContext`` and before/after executing passes.
Copy link
Contributor

Choose a reason for hiding this comment

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

provide callbacks to run

Copy link
Contributor

Choose a reason for hiding this comment

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

Since PassInstrument is an interface, it might be more helpful to speak about the interface as a whole rather than mention callbacks, which are just one way of implementing the interface. e.g.

The PassInstrument interface allows you to run code when entering/exiting PassContext and before ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

As more and more passes are implemented, it becomes interesting to instrument
passes execution, analyze per-pass effects and observe various events.
We have extended :py:class:`tvm.transform.PassContext` to accept a list of
instrument classes. Also a decorator :py:func:`tvm.ir.instrument.pass_instrument` is provided to easily implement instrument classes.
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, you are right.

So it should be,
Pass instrument framework extended ... instrument instances, Also a decorator :py:func:tvm.instrument.pass_instrument ... instrument classes.
Sorry for miss-copied the second one.

instrument classes. Also a decorator :py:func:`tvm.ir.instrument.pass_instrument` is provided to easily implement instrument classes.

This tutorial demostrates how developers can use ``PassContext`` to instrument
passes. For more details, please refer to the :ref:`pass-infra`
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems no change.

@zackcquic
Copy link
Contributor

zackcquic commented Jun 11, 2021

Thanks for prompt feedbacks @zackcquic @tkonolige !

Here are some comments for Zack's questions:

  1. What happens when exceptions occur in different instrument point.
    Added in pass_infra.txt. But it is a little long. You might want to take a look again.
  2. Standard Instrument section: PassTimingInstrument, PrintBefore(TODO), PrintAfter(TODO), ..
    I think it might be better to maintain these in the doc string of related Python class/function.
    So I add example to instrument.py. It will be shown in Python API reference.
  3. Global PassContext and override_instrument examples
    Done. Sorry for not aware of this approach.
  4. use_pass_infra.py's comments should be updated, sorry, I forgot to update it.
    Done.
  5. conf.py should be updated.
    Done. But actually it seems to automatically append un-listed tutorials to the end.
    How do you think about the current order of tutorial?

Thanks a lot for the hard work, its very helpful. @chiwwang

For 2, I think it should be mentioned in this doc, like vm optimizations
If maybe a little hard to find (eg grep the whole source without knowing the name).

For 3, I think it needs a little section:
Since passes can be used without explicit PassContext, so we introduced override...
// ... more description

For 5. I would like to group pass related together, it seems done in the latest fix.

Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

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

thanks for contributing @chiwwang ! added some comments inline.

Pass Instrument
~~~~~~~~~~~~~~~

``PassInstrument`` provides callbacks run when entering/exiting ``PassContext`` and before/after executing passes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since PassInstrument is an interface, it might be more helpful to speak about the interface as a whole rather than mention callbacks, which are just one way of implementing the interface. e.g.

The PassInstrument interface allows you to run code when entering/exiting PassContext and before ...


``PassInstrument`` provides callbacks run when entering/exiting ``PassContext`` and before/after executing passes.
Multiple ``PassInstrument`` instances can be registed into a single ``PassContext``.
Instrument instances are called sequentially in the order of ``instruments`` argument passed to ``PassContext``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't abbreviate class names, it will get very confusing: s/Instrument/PassInstrument/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


} // namespace instrument

Python interfaces are provided to implement ``PassInstrument`` quickly.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it would be good to include a pointer to these, since a pointer to the C++ ones are above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I add a new tag for python frontend here. Thanks.

This method is also called when instruments is being overriden by ``override_instruments`` in ::py:class:`tvm.transform.PassContext`.

``InstrumentBeforePass`` is called before pass-execution.
``InstrumentAfterPass`` is called after pass-executioon if the pass should be run. The behavir is like:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: execution, behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


- ``InstrumentBeforePass``

* ``ShouldRun`` callbakc is executed if the pass is not listed as a required pass.
Copy link
Contributor

Choose a reason for hiding this comment

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

i think you could just delete the word callback here. ShouldRun is an interface method.

@@ -389,6 +396,103 @@ To allow other C++ modules to apply this pass, we declare a free function in

TVM_DLL Pass FoldConstant();

.. _pass_instrument_section_tag:

Pass Instrument
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems like PassContext may be introduced after this section, no? Perhaps it makes sense to move it downward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed wordings.
As for the order of introduction, It's a little embarrassing. I would like to put all Pass Instrument descriptions together and after PassContext introduction.
But the pass_infra.txt is divided to C++ backend and Python Frontend parts.
I tried to follow that. So the order of introduction becomes:

  1. PassContext C++ backend
  2. PassInstrument C++ backend
  3. PassContext Python frontend
  4. PassInstrument Python frontend

The PassContext Python frontend is after PassInstrument C++ backend.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah i see. what do you think about documenting the PassInstrument first, since that's the developer-facing thing? TVM calls InstrumentEnterPassContext.

i think explaining the C++ thing first and then the Python thing is workable. i agree it's a bit tricky to work around the split. I don't think the Python docs need to be as extensive since they mostly wrap the C++ stuff

Copy link
Contributor Author

@chiwwang chiwwang Jun 28, 2021

Choose a reason for hiding this comment

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

It would be nice! But after trying and reading around, I find PassInstrument might require understanding PassContext first to know about what each instrument points means.
Perhaps we can split PassInstrument doc out once it becomes bigger (such as more and more built-in instruments are added, or new functionality added to provide more instrumentation possibility?)

I also tried to shrink Python docs a little. Thanks a lot for suggestions @areusch :)

relay_mod["main"] = bind_params_by_name(relay_mod["main"], relay_params)
# timing_inst is put after call_node_inst.
# So the execution time of ``call_node.inst.run_after_pass()`` is also counted.
with tvm.transform.PassContext(opt_level=3, instruments=[call_node_inst, timing_inst]):
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be super awesome to add another section demonstrating an exception case here!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added. Thanks for suggestions!

@chiwwang
Copy link
Contributor Author

chiwwang commented Jun 14, 2021

Ah! I saw what confused me. It's the level of "Pass Registration" section.
The current pass_infra.rst section-hierachy is

Pass Infrastructure (Topmost)

  • The Design
    The design of backend and frontend are described here.
    • C++ Backend
      • PassContext
      • Pass Constructs
      • Module-Level Passes
      • Function-Level Passes
      • Sequential Passes
    • Pass Registration <-----This section has the same level with Backend/Frontend.
    • Python Frontend
      • PassContext
      • Pass Objects

Now I add Pass Instrument as:

Pass Infrastructure (Topmost)

  • The Design
    The design of backend and frontend are described here.
    • C++ Backend
      • PassContext
      • Pass Constructs
      • Module-Level Passes
      • Function-Level Passes
      • Sequential Passes
      • Pass Registration <----- May I fix this to have the same level with other sub-sections in C++ backend?
      • Pass Instruments <--- Added in this PR.
      • Built-in Instrument <--- Added in this PR.
    • Python Frontend
      • PassContext
      • Pass Objects
      • Pass Instrument <--- Added in this PR.
      • Override Instruments in Current PassContext <--- Added in this PR.

This might looks matching with descriptions in "The Design" section.
Or, could we isolate Pass Instrument, and have another topmost section as Pass Infrastructure?
May I know your thoughts @zackcquic @areusch ?
Thanks a lot!

):
# ...

One can implement a ``PassInstrument`` by using the ``pass_instrument`` decorator(`python/tvm/ir/instrument.py`_) on a class implementing following methods:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Maybe it should be emphasized to use decorator, instead of overriding/subclassing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Pass Instrument
^^^^^^^^^^^^^^^

Currently we introduce four instrument point in the life-cycle of ``PassContext``.
Copy link
Contributor

Choose a reason for hiding this comment

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

instrument points

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

):
# ...

One can implement a ``PassInstrument`` by using the ``pass_instrument`` decorator(`python/tvm/ir/instrument.py`_) on a class implementing following methods:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think it maybe better to emphasize use decorator instead of subclassing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@zackcquic
Copy link
Contributor

Ah! I saw what confused me. It's the level of "Pass Registration" section.
The current pass_infra.rst section-hierachy is

Pass Infrastructure (Topmost)

  • The Design
    The design of backend and frontend are described here.

    • C++ Backend

      • PassContext
      • Pass Constructs
      • Module-Level Passes
      • Function-Level Passes
      • Sequential Passes
    • Pass Registration <-----This section has the same level with Backend/Frontend.

    • Python Frontend

      • PassContext
      • Pass Objects

Now I add Pass Instrument as:

Pass Infrastructure (Topmost)

  • The Design
    The design of backend and frontend are described here.

    • C++ Backend

      • PassContext
      • Pass Constructs
      • Module-Level Passes
      • Function-Level Passes
      • Sequential Passes
      • Pass Registration <----- May I fix this to have the same level with other sub-sections in C++ backend?
      • Pass Instruments <--- Added in this PR.
      • Built-in Instrument <--- Added in this PR.
    • Python Frontend

      • PassContext
      • Pass Objects
      • Pass Instrument <--- Added in this PR.
      • Override Instruments in Current PassContext <--- Added in this PR.

This might looks matching with descriptions in "The Design" section.
Or, could we isolate Pass Instrument, and have another topmost section as Pass Infrastructure?
May I know your thoughts @zackcquic @areusch ?
Thanks a lot!

Just know this PR put all in Pass Infrastructure
I previous assume this a separate section like Pass Infrastructure with the sequence:

  1. Introduce what is a pass instrument
    1. instrument points
  2. PassContext and multiple instance details
  3. Examples

But this PR's organization is ok for me.
Wait for @areusch @tkonolige to see if they have more comments.

@chiwwang
Copy link
Contributor Author

Gentle ping to see whether we have any comment on this change. Thanks!
@areusch @tkonolige @ZihengJiang

Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

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

thanks for the ping and sorry for the delay @chiwwang ! just a few more comments.

Pass Instrument
^^^^^^^^^^^^^^^

Currently we introduce four instrument points in the life-cycle of ``PassContext``.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe could you add a short sentence explaining what a "Pass Instrument" is? e.g. what does it mean to "instrument" the pass manager?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks!


There are several built-in instruments. Those marked with *TODO* are not implemented yet.

PassTimmingInstrument (see `src/ir/instrument.cc`_)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Timing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks!

@@ -389,6 +396,103 @@ To allow other C++ modules to apply this pass, we declare a free function in

TVM_DLL Pass FoldConstant();

.. _pass_instrument_section_tag:

Pass Instrument
Copy link
Contributor

Choose a reason for hiding this comment

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

ah i see. what do you think about documenting the PassInstrument first, since that's the developer-facing thing? TVM calls InstrumentEnterPassContext.

i think explaining the C++ thing first and then the Python thing is workable. i agree it's a bit tricky to work around the split. I don't think the Python docs need to be as extensive since they mostly wrap the C++ stuff


PassTimmingInstrument (see `src/ir/instrument.cc`_)

PrintBefore(TODO)
Copy link
Contributor

Choose a reason for hiding this comment

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

could you give a short explanation what these are?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

See :ref:`pass_instrument_overriden`.

``InstrumentBeforePass`` is called before execution.
``InstrumentAfterPass`` is called after executioon if the pass should be run. The behavior is like:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: execution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks!

@areusch
Copy link
Contributor

areusch commented Jun 28, 2021

thanks @chiwwang , please address the CI failure:

/workspace/docs/tutorials/dev/use_pass_infra.rst:307: WARNING: undefined label: pass_instrument_section_tag

Examples
--------

The following code-block shows how to use this instrument.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just delete this line.


Examples
--------
The following code block decorates a pass instrument class.
The following code block shows how to decorate a pass instrument class.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can delete this line.

**Author**: `Chi-Wei Wang <https://github.com/chiwwang>`_

As more and more passes are implemented, it becomes useful to instrument
passes execution, analyze per-pass effects and observe various events.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
passes execution, analyze per-pass effects and observe various events.
pass execution, analyze per-pass effects, and observe various events.

I think we generally use the Oxford comma?

Comment on lines 27 to 30
Pass infrastructure provides instrument mechanism. One can pass a list of
instrument instances to :py:class:`tvm.transform.PassContext`.
Also a decorator :py:func:`tvm.instrument.pass_instrument` is provided
to easily implement instrument classes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Pass infrastructure provides instrument mechanism. One can pass a list of
instrument instances to :py:class:`tvm.transform.PassContext`.
Also a decorator :py:func:`tvm.instrument.pass_instrument` is provided
to easily implement instrument classes.
We can instrument passes by providing a list of :py:class:`tvm.ir.instrument.PassInstrument` instances to :py:class:`tvm.transform.PassContext`. We provide a pass instrument for collecting timing information (:py:class:`tvm.ir.instrument.PassTimingInstrument`), but an extension mechanism is available via the :py:func:`tvm.instrument.pass_instrument` decorator.

image_shape = (3, 224, 224)
output_shape = (batch_size, num_of_image_class)
relay_mod, relay_params = resnet.get_workload(num_layers=18, batch_size=1, image_shape=image_shape)
print(relay_mod.astext(show_meta_data=False))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a print statement before this that mentions what we are printing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


###############################################################################
# If an exception occur in ``enter_pass_ctx``, ``PassContext`` disable the pass
# instrumentation. And it will run ``exit_pass_ctx`` of each ``PassInstrument``
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# instrumentation. And it will run ``exit_pass_ctx`` of each ``PassInstrument``
# instrumentation. And it will run the ``exit_pass_ctx`` of each ``PassInstrument``

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 300 to 301
# Also, all ``PassInstrument`` are cleared.
# So nothing printed while ``override_instruments`` is called.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Also, all ``PassInstrument`` are cleared.
# So nothing printed while ``override_instruments`` is called.
```suggestion
# Exceptions in ``PassInstrument``s cause all instruments of the current ``PassContext`` to be cleared, so nothing is printed when ``override_instruments`` is called.

Copy link
Contributor Author

@chiwwang chiwwang Jun 29, 2021

Choose a reason for hiding this comment

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

Done. But the 's' in "PassInstruments" seems to cause rendering problems. (Sphinx 3.5.4)
I changed it to PassInstrument instances.

demo_ctx.override_instruments([]) # no PassFine_0 exit_pass_ctx printed....etc

###############################################################################
# If an exception occur in ``exit_pass_ctx``, pass instrumentation is disabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# If an exception occur in ``exit_pass_ctx``, pass instrumentation is disabled.
# If an exception occurs in ``exit_pass_ctx``, then the pass instrument is disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


###############################################################################
# If an exception occur in ``exit_pass_ctx``, pass instrumentation is disabled.
# Then exception is thrown. That means ``PassInstrument`` registered
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Then exception is thrown. That means ``PassInstrument`` registered
# This exception is propagated. ``PassInstrument``s registered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


###############################################################################
# Exceptions occured in ``should_run``, ``run_before_pass``, ``run_after_pass``
# are not handled explicitly -- that means, we rely on the context manager
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# are not handled explicitly -- that means, we rely on the context manager
# are not handled explicitly -- we rely on the context manager

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@chiwwang
Copy link
Contributor Author

chiwwang commented Jun 29, 2021

@areusch Oops sorry I forgot to change the tag name. Fixed and checked with task_sphinx_precheck.sh. Hope it works this time.

@tkonolige Thank you for so detailed review!
And sorry for my some careless grammar mistakes :(

@areusch
Copy link
Contributor

areusch commented Jun 29, 2021

@tkonolige @zackcquic please take a look and explicitly approve if you're good with this PR

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.

@chiwwang Everyone makes grammar mistakes, thats why we have code review :)

Copy link
Contributor

@zackcquic zackcquic left a comment

Choose a reason for hiding this comment

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

LGTM

@chiwwang
Copy link
Contributor Author

chiwwang commented Jul 5, 2021

Thanks @zackcquic @tkonolige !!
@areusch you might want to check if this PR is ready for merging. Thanks :)

@areusch areusch merged commit a4775c2 into apache:main Jul 7, 2021
@areusch
Copy link
Contributor

areusch commented Jul 7, 2021

thanks @chiwwang, the PR is now merged!

@chiwwang
Copy link
Contributor Author

chiwwang commented Jul 8, 2021

Thanks you all!! I learn so much from all of feedbacks :)

ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
* Fix AttributeError when TEST_DATA_ROOT_PATH is set

Initiate a Path object from TEST_DATA_ROOT_PATH to fix the error:
AttributeError: 'str' object has no attribute 'mkdir'

* [DOCS] Add docs for Pass Instrument

 - Add a tutorial about how to use pass instrument.
 - Add related sections in Pass Infrastructure documents.

* Fix ir.rst, the length of separator.

* Fix unused local name

* Fix linting errors

* Fix linting errors

* Fix linting errors

* Address code-review feedbacks

* Fix linting

* Fix the order of tutorial.

* Add exception handling. Address feedbacks.

* Fix CI error -- clearing instruments in global pass_ctx

* Clarify section hierachy.

* Emphasize to use decorator instead of subclassing

* Add a sentence to explain Pass Instrument. Fix typo.

* Shrink python docs a little.

* Fix tag name.

* Address feedbacks.
zxy844288792 pushed a commit to zxy844288792/tvm that referenced this pull request Mar 4, 2022
* Fix AttributeError when TEST_DATA_ROOT_PATH is set

Initiate a Path object from TEST_DATA_ROOT_PATH to fix the error:
AttributeError: 'str' object has no attribute 'mkdir'

* [DOCS] Add docs for Pass Instrument

 - Add a tutorial about how to use pass instrument.
 - Add related sections in Pass Infrastructure documents.

* Fix ir.rst, the length of separator.

* Fix unused local name

* Fix linting errors

* Fix linting errors

* Fix linting errors

* Address code-review feedbacks

* Fix linting

* Fix the order of tutorial.

* Add exception handling. Address feedbacks.

* Fix CI error -- clearing instruments in global pass_ctx

* Clarify section hierachy.

* Emphasize to use decorator instead of subclassing

* Add a sentence to explain Pass Instrument. Fix typo.

* Shrink python docs a little.

* Fix tag name.

* Address feedbacks.
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.

6 participants