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

Add API docs for ONNX Runtime and Intel #100

Merged
merged 27 commits into from
Mar 23, 2022
Merged

Add API docs for ONNX Runtime and Intel #100

merged 27 commits into from
Mar 23, 2022

Conversation

lewtun
Copy link
Member

@lewtun lewtun commented Mar 17, 2022

What does this PR do?

This PR adds the API docs for the intel and onnxruntime packages. It depends on the fixes to doc-builder in this PR, so the CI should pass once that's merged.

The docs are structured as follows:

docs
└── source
    ├── index.mdx  # The landing page / README
    ├── intel  # API docs go under their own subpackage
    │   ├── configuration.mdx  # I use the same naming conventions across subpackages
    │   ├── optimization.mdx
    │   ├── pruning.mdx
    │   ├── quantization.mdx
    │   └── trainer.mdx
    ├── onnxruntime
    │   ├── configuration.mdx
    │   ├── optimization.mdx
    │   ├── quantization.mdx
    │   └── trainer.mdx
    ├── quickstart.mdx  # Guides go in the root of source/
    └── _toctree.yml  # The table of contents

While putting together the docs, I ran into a few optimum quirks that I would like feedback on:

  1. Unlike transformers, optimum doesn't have a dev install that includes the intel and onnxruntime packages. Is this by design? Would it make sense to have pip install '.[dev]' install all subpackages?
  2. Some of the modules have inconsistent naming across intel and onnxruntime. For example, we have intel.neural_compressor.optimizer and onnxruntime.optimization. I've standardised this in the docs to have an "optimization" section per subpackage, and am happy to rename the intel module name if you want.
  3. To build the docs, I had to include the intel.neural_compressor imports in the root init of the intel subpackage. Is this OK?

@lewtun lewtun marked this pull request as ready for review March 17, 2022 21:33
@@ -48,7 +48,7 @@ jobs:
cd ..

cd optimum
pip install .[dev]
pip install .[dev,onnxruntime,intel]
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 have to include all the dependencies here to build the docs

@@ -11,3 +11,9 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

from .neural_compressor.config import IncConfig, IncPruningConfig, IncQuantizationConfig
Copy link
Member Author

Choose a reason for hiding this comment

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

Here I have to include the neural_compressor imports in the root init of the intel subpackage to build the docs


## IncOptimizer

[[autodoc]] intel.neural_compressor.optimizer.IncOptimizer
Copy link
Member Author

@lewtun lewtun Mar 17, 2022

Choose a reason for hiding this comment

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

For consistency with the onnxruntime package, I suggest we rename this module to intel.neural_compressor.optimization. I realise this is a breaking change, but I think it will help optimum users more easily navigate the API.

We can discuss this in a separate issue / thread since it doesn't relate to this PR very strongly

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that's a good point, I was also thinking to rename intel.neural_compressor.config to intel.neural_compressor.configuration for the same reason. I agree that this is an important change that should be done as soon as possible (before our next release, which should be soon)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK great, let's tackle that in a separate PR :)


## IncOptimizer

[[autodoc]] intel.neural_compressor.optimizer.IncOptimizer
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that's a good point, I was also thinking to rename intel.neural_compressor.config to intel.neural_compressor.configuration for the same reason. I agree that this is an important change that should be done as soon as possible (before our next release, which should be soon)

Comment on lines 13 to 17
# Trainer

## IncTrainer

[[autodoc]] intel.neural_compressor.trainer_inc.IncTrainer
Copy link
Collaborator

Choose a reason for hiding this comment

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

There seems to be a mismatch with docs/source/intel/trainer.mdx

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I wasn't sure whether to use the same hierarchy as we have in the onnxruntime docs. Happy to nest it though so it lines up with the source code!

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I meant is that IncTrainer is displayed in configuration.mdx whileIncOptimizer is displayed in trainer.mdx

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 catch, fixed now!

@echarlaix
Copy link
Collaborator

Great addition, thanks a lot @lewtun !!

@lewtun lewtun mentioned this pull request Mar 22, 2022
@@ -48,7 +48,7 @@ jobs:
cd ..

cd optimum
pip install .[dev,onnxruntime,intel]
pip install -e .[dev,onnxruntime,intel]
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I am not very knowledgeable on the subject, but why do we need to install the packages in development mode?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, that was me trying to debug the doc-builder in the GitHub actions - it's removed now!

@@ -88,6 +88,7 @@ jobs:
NODE_OPTIONS: --max-old-space-size=6656
run: |
cd doc-build-dev && git pull
python -c "from optimum import intel; print(dir(intel))"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a temporary debug for the GH actions - will remove before merging

@lewtun lewtun merged commit 4a83d95 into main Mar 23, 2022
@lewtun lewtun deleted the add-api-docs branch March 23, 2022 15:35
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.

3 participants