Skip to content

Conversation

@arkq
Copy link
Contributor

@arkq arkq commented Oct 15, 2025

NNFW types and numpy data types do not map one to one because NNFW has quantized types represented as uint8 or int16. Because of that it should be more efficient to export custom data type object which will map these two types.

Additionally, for convenience, dedicated types are exported in the top-level onert Python module, so one can use them as follows:

>>> import numpy as np
>>> import onert
>>> onert.
onert.bool          onert.dtype(        onert.float32       onert.int32         onert.native        onert.qint8         onert.tensorinfo(
onert.common        onert.experimental  onert.infer         onert.int64         onert.qint16sym     onert.quint8        onert.uint8
>>> np.array([2, 42, 42], dtype=onert.float32)
array([ 2., 42., 42.], dtype=float32)

ONE-DCO-1.0-Signed-off-by: Arkadiusz Bokowy a.bokowy@samsung.com

@arkq arkq force-pushed the python-onert-dtypes branch from 6f78932 to c36e868 Compare October 15, 2025 14:52
@ragmani
Copy link
Contributor

ragmani commented Oct 17, 2025

While reviewing this PR I noticed a couple of pre-existing issues (not introduced by these changes):

  1. Quantization info
    There is currently no clear mechanism for handling quantization parameters (scale, zero_point). We probably need a consistent way to represent and preserve these attributes.

  2. Data type mapping between onert and numpy
    At the moment, there is no standard mapping layer between onert.dtype and numpy.dtype. This means patterns like

np.array([1, 2, 3], dtype=onert.qint8)

will not work as expected. We likely need a bidirectional converter and clear usage rules so that dtype handling is both intuitive and reliable.

These are not regressions introduced by this PR, but they directly affect how usable the Python API works. If possible, could you also consider addressing them when binding tensorinfo?

@ragmani
Copy link
Contributor

ragmani commented Oct 17, 2025

One additional request: after the changes in this PR, could you please check that the existing python examples still run correctly? Currently our CI does not include python API validation.

@arkq
Copy link
Contributor Author

arkq commented Oct 22, 2025

At the moment, there is no standard mapping layer between onert.dtype and numpy.dtype. This means patterns like
np.array([1, 2, 3], dtype=onert.qint8)
will not work as expected. We likely need a bidirectional converter and clear usage rules so that dtype handling is both intuitive and reliable.

I've been thinking about extending numpy with OneRT quantized types, so then numpy will be able to convert between floats, integers and quantized types. I've never done that before though, so I will have to investigate the pros and cons of such approach. Anyway, I think that then the API will be clear and simple because it would be possible to use all numpy operations (hopefully) on quantized tensors.

@ragmani
Copy link
Contributor

ragmani commented Oct 23, 2025

I also looked into this and found NumPy’s ongoing work on a new dtype system relevant here:

https://numpy.org/neps/roadmap.html#extensibility

  • NEP 41: outlines the shortcomings of the old monolithic dtype system and sets the direction for a modular, extensible design.
  • NEP 42: proposes the new DType class hierarchy and APIs so that user-defined dtypes (e.g. quantized types with scale/zero-point) can be registered cleanly.
  • NEP 43: extends universal functions (ufuncs) to integrate seamlessly with new dtypes, so user-defined dtypes can participate in NumPy operations.

All three are accepted or under discussion, but full implementation is still ongoing. Until this new dtype system is complete, introducing something like onert.qint8 directly into NumPy is not realistic.

Given this situation, I would suggest some practical options for now:

  • Provide a mapping/conversion layer between onert.dtype and numpy.dtype in the Python bindings to ensure usability.
  • If that turns out to be too complex or fragile, document it explicitly as a known issue and defer full support until NumPy’s extensible DType framework matures.

ragmani
ragmani previously approved these changes Oct 23, 2025
Copy link
Contributor

@ragmani ragmani left a comment

Choose a reason for hiding this comment

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

LGTM

@arkq
Copy link
Contributor Author

arkq commented Oct 23, 2025

LGTM

Maybe we should wait with that PR (at least until the next week)? I will also look at the possibility to extend numpy data types, because I've seen somewhere (some time ago) that it is possible but maybe not very easy to do. I will investigate how complicated it would be right now (if possible at all). I will submit my investigation result here as well.

@ragmani
Copy link
Contributor

ragmani commented Oct 23, 2025

That sounds like a good idea. I’ll look forward to your investigation.

@hseok-oh
Copy link
Contributor

@arkq Please resolve conflicts

@hseok-oh
Copy link
Contributor

hseok-oh commented Nov 5, 2025

@arkq Please resolve conflict
@tomdol Please review this PR

@arkq
Copy link
Contributor Author

arkq commented Dec 1, 2025

@ragmani, I've researched a little bit the possibility of adding custom dtype directly to numpy and the conclusion is that as of now it's not possible to add a type which is extensible enough to cover quantized type. For quantized type we would need a type which allows to set scaling factor and the zero-point (for asymmetrical quantization) and stores the value of course. The case is, that it is possible to create a type which let say is scaled by 1000 and has a zero point at 50, but it would have to be a statically registered type with pre-defined scaling and zero point.... With current numpy API it's not possible to overcome that design flaw. I thought that maybe somehow it would be possible to create a parametric type with current API, but unfortunately it is not possible, because conversion functions get type "class instance", so parametrization on the fly is not possible.

tl;dr; your previous post is correct, numpy needs at least some of the features described in https://numpy.org/neps/nep-0042-new-dtypes.html#nep42 (i.e. type's parameters) to allow quantized type

@arkq arkq force-pushed the python-onert-dtypes branch from 1ed7bd5 to cb78e0d Compare December 1, 2025 12:11
@ragmani
Copy link
Contributor

ragmani commented Dec 2, 2025

@arkq
Thanks a lot for looking into the detailed investigation and the effort. I may not be able to keep tracking the part of onert development closely going forward due to an issue on my side.
Anyway, given the current limitations in NumPy’s dtype system, one possible approach might be to document this as a known issue for now and defer full qint8 dtype support until a more suitable mechanism becomes available.

@ragmani
Copy link
Contributor

ragmani commented Dec 2, 2025

Is this PR ready for review, now?

@arkq
Copy link
Contributor Author

arkq commented Dec 2, 2025

Is this PR ready for review, now?

Yes, it's ready

@glistening
Copy link
Contributor

@arkq I looked the code to find what is the change in user's side. However, I cannot find the changes in example codes. The changes in minimal.py and other seem irrelevant.

arkq added 2 commits December 3, 2025 13:59
NNFW types and numpy data types do not map one to one because NNFW has
quantized types represented as uint8 or int16. Because of that it should
be more efficient to export custom data type object which will map these
two types.

Additionally, for convenience, dedicated types are exported in the
top-level onert Python module, so one can use them as follows:

> import numpy as np, onert
> np.array([2, 42, 42], dtype=onert.float32)

ONE-DCO-1.0-Signed-off-by: Arkadiusz Bokowy <a.bokowy@samsung.com>
@arkq
Copy link
Contributor Author

arkq commented Dec 3, 2025

@glistening, there are no changes in examples because all examples use dtype from tensorinfo when creating numpy arrays. Anyway, I've just grepped the code and I've found that I should update dtypes in the onert Python module...

Copilot AI review requested due to automatic review settings December 3, 2025 15:30
@arkq arkq force-pushed the python-onert-dtypes branch from cb78e0d to acec594 Compare December 3, 2025 15:30
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request refactors the OneRT Python API's type system by introducing dedicated data type objects that properly map between NNFW tensor types and numpy dtypes. Previously, types were represented as strings, which didn't adequately handle quantized types (e.g., qint8, quint8) that share underlying numpy dtypes (int8, uint8) with non-quantized types.

Key Changes:

  • Replaced string-based type conversion with a new datatype C++ class that encapsulates NNFW_TYPE and its corresponding numpy dtype
  • Exposed convenient top-level type objects (onert.float32, onert.int32, onert.qint8, etc.) for easy usage with numpy
  • Updated the DataLoader default parameter to use onert.float32 instead of np.float32 for consistency

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
runtime/onert/api/python/include/nnfw_api_wrapper.h Added datatype struct to encapsulate NNFW_TYPE with numpy dtype; removed old string-based conversion function declarations; updated tensorinfo to use datatype instead of string
runtime/onert/api/python/src/wrapper/nnfw_api_wrapper.cc Implemented datatype constructor with switch-case mapping for all NNFW tensor types; removed getType/getStringType functions; updated all type conversions to use datatype class; simplified getLayout control flow
runtime/onert/api/python/src/bindings/nnfw_tensorinfo_bindings.cc Added pybind11 bindings for datatype class with operator overloading and property exposure; created dtypes submodule to export all type constants
runtime/onert/api/python/onert/init.py Updated imports to expose dtype and tensorinfo from native bindings; added wildcard import from dtypes submodule to expose type constants at top level
runtime/onert/api/python/onert/experimental/train/dataloader.py Changed default dtype parameter from np.float32 to onert.float32 for consistency with the new type system

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@glistening
Copy link
Contributor

glistening commented Dec 4, 2025

@glistening, there are no changes in examples because all examples use dtype from tensorinfo when creating numpy arrays. Anyway, I've just grepped the code and I've found that I should update dtypes in the onert Python module...

My understanding is:

@arkq
Copy link
Contributor Author

arkq commented Dec 4, 2025

My understanding is:

Yes, that's correct. Beside the simple test in the PR description, I've tested this PR also with:

python runtime/onert/sample/minimal-python/minimal.py nnpackage/examples/v1.3.0/two_tflites/mv1.0.tflite

The minimal.py uses dtype from tensorinfo to create all-zeros array. Previously the dtype exposed by tensorinfo was numpy.dtype, but now it is onert.dtype and nothing is broken. So, this test also verifies the correctness of changes in this PR. To test changes in the onert.experimetan.train I've run runtime/onert/sample/minimal-python/experimental/src/train_step_with_dataset.py script, and it also works as expected:

$ runtime/onert/sample/minimal-python/experimental/src/train_step_with_dataset.py \
    -m nnpackage/examples/v1.3.0/two_tflites/mv1.0.tflite \
    --input=dataset.npy --label=labels.npy --data_length=1 --batch_size=16
Load data
== training parameter ==
- learning_rate = 0.01
- batch_size = 16
- loss_info = {loss = MeanSquaredError, reduction = sum over batch size}
- optimizer = SGD
- num_of_trainable_ops = -1
========================
Step 1/1 - Train time: 289.558 ms/step - Train Loss: 3.4727
===================================
Average Loss: 3.4727
CategoricalAccuracy: 0.0000
Average Time: 289.5583 ms/step
===================================
nnpackage mv1.0.tflite trains successfully.

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.

5 participants