Upgrade ONNX runtime, ONNX Gen AI runtime, add a new model to test changes#1085
Upgrade ONNX runtime, ONNX Gen AI runtime, add a new model to test changes#1085ammbra wants to merge 5 commits into
Conversation
Signed-off-by: Ana-Maria Mihalceanu <ana-maria.m.mihalceanu@oracle.com>
|
👋 Welcome back amihalceanu! A progress list of the required criteria for merging this PR into |
|
@ammbra This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 2 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the As you do not have Committer status in this project an existing Committer must agree to sponsor your change. ➡️ To flag this PR as ready for integration with the above commit message, type |
Webrevs
|
| } | ||
|
|
||
| static TensorProto tensorProto(String name, oracle.code.onnx.Tensor tensor, Function<Tensor, ExternalTensorDataInfo> tensorDataExternalizer) { | ||
| private static List<Object> determineLlmShape(String name) { |
There was a problem hiding this comment.
Could we please avoid hardcoding tensor dimensions in OnnxProtoBuilder? This would work only for one concrete model.
If the shape information is missing from the ONNX maybe we can pass this metadata from the GenAI/demo layer now or find better solution in a follow-up PR.
There was a problem hiding this comment.
As a temporary solution we can use for example an interface with concrete shapes:
interface OnnxShapeHints {
Map<String, double[]> onnxShapeHints();
}
A rock solid solution would be to implement shape inference.
There was a problem hiding this comment.
I added a factory for shape inference. Can you please take a look at the new way to handle it?
| <|start_header_id|>user<|end_header_id|>Hello, tell me a joke.<|eot_id|> | ||
| <|start_header_id|>assistant<|end_header_id|> | ||
| """, System.out::print); | ||
| Reader inreader = new InputStreamReader(System.in); |
There was a problem hiding this comment.
Is more interactive, you can have a chat conversation in command line. But if you believe would be best to keep it simple, I can revert it.
There was a problem hiding this comment.
It might be interactive, however it requires user to wrap simple text prompt into Llama-specific form manually.
There was a problem hiding this comment.
Oh, I read more here and I understand what you mean now. While the easiest way would be to go back, would prompt formatting here be ok to keep it interactive? One drawback I find to prompt formatting is that sanitising the input would be needed. 🙏
Signed-off-by: Ana-Maria Mihalceanu <ana-maria.m.mihalceanu@oracle.com>
Signed-off-by: Ana-Maria Mihalceanu <ana-maria.m.mihalceanu@oracle.com>
Signed-off-by: Ana-Maria Mihalceanu <ana-maria.m.mihalceanu@oracle.com>
Signed-off-by: Ana-Maria Mihalceanu <ana-maria.m.mihalceanu@oracle.com>
There was a problem hiding this comment.
I’m still working through all the proposal, so please correct me if I misunderstand something.
There is clearly a need for named inputs and outputs in the graph. This is already handled inside OnnxTransformer, where ModuleAndInitializers holds the name map, allowing method parameter names and return record component names to be forwarded to the ONNX model.
The proposal seems to go in the opposite direction: to apply names parsed from the config file. I’m a bit concerned about this, because it moves us away from the idea that Java should remain the source of truth for the model.
There is also a clear need to provide shape information for some ONNX operations. This is new and would require passing additional information into the protobuf-building process. There are two parts to this issue:
- identify the op input that requires an explicit shape
- provide the actual shape value
If I understand the proposal correctly, the tensor would be identified by its name coming from the config, and the shape value would be represented as a list of objects, mixing literal values with references to the configuration file or even references to internal runtime values. My concern is that none of this information comes directly from the Java source.
If the shape definitions are primarily needed for method parameters, then the mapping key should probably be the tensor method parameter, or more generally some code-reflection value. If we had code reflection support for value attributes, or if we had support for parameter annotations, this could be handled more cleanly. Since we are not there yet, some workaround may be necessary. For example using parameter names as keys for the tensor shape resolver.
For the second part (the shape itself) I think it should still be Java-sourced. In practice, that would mean resolving it to an array of longs. Otherwise we risk introducing unclear references to “magic” constants coming from external configuration.
When I refer to shape inference, I mean that if shape information is required deeper in the model tree, it should ideally be inferred from the parameters and weights through the operations in the tree. Since that is a more complex task, we could temporarily rely on a Java variable-name-based mapping, even for values deeper in the model. Another option would be to identify the ops that require shape information and add synthetic shape attributes to them, in addition to their specs-generated attributes, so the shape constants are still passed explicitly from Java.
| <|start_header_id|>user<|end_header_id|>Hello, tell me a joke.<|eot_id|> | ||
| <|start_header_id|>assistant<|end_header_id|> | ||
| """, System.out::print); | ||
| Reader inreader = new InputStreamReader(System.in); |
There was a problem hiding this comment.
It might be interactive, however it requires user to wrap simple text prompt into Llama-specific form manually.
| case String shapeName when (shapeName.matches("past(Key|Value)\\.\\d+")) -> List.of("batch_size", keyValueHeads, "past_sequence_length", headSize); | ||
| case String shapeName when (shapeName.matches("present(Key|Value)\\.\\d+")) -> List.of("batch_size", keyValueHeads, "total_sequence_length", headSize); | ||
| case String _ -> null; | ||
| }; |
There was a problem hiding this comment.
This should break into if/else blocks, pattern matching does not make sense here.
|
|
||
| import java.util.List; | ||
|
|
||
| public record NonTensorShapeResolver() implements TensorShapeResolver { |
There was a problem hiding this comment.
Why there is a custom resolver resolving to nothing?
I apologize for such a big PR, but most of its changes are result of regeneration for native bindings, operators, prototufs etc. So, this PR contains many changes that relate to upgrading the ONNX runtime library (1.26.0) and its native GenAI (0.14.0) counterpart:
oracle.code.onnx.foreignare generated via the cr-examples/onnx/opgen/setup.sh script and its helper symbols files to generate only the absolute necessary contents.OnnxProtoBuilderneeded to support also shapes because I kept getting this native error once rebuilding Babylon:I managed to identify that the exported model was the source for such an error (both local debug and by also comparing in Netron). To my understanding,
Generators::Model::IsPruned() constmethod checks if the underlying model has been compressed using model pruning. Pruning seems to reduce model size and latency by zeroing out or permanently removing unnecessary weights and parameters, but I think in this case the generated model was aggressively pruned from the beginning of traversing the graph, thus resulting in the error above. The aggressive pruning was performed due the lack of shapes being there. This error occurred only for LLMs, the rest of the ML models being unaffected.AllMiniLML6V2EmbeddingModelis inspired by https://huggingface.co/onnx-community/all-MiniLM-L6-v2-ONNX and by running theOnnxLiftover its default (model.onnx). As a result, I could obtain a quite good model to start. Then I cleaned it locally based on how the official config.json looked like. TheBertTokenizeris just a helper for working with the tokenizer and its constants are from tokenizer_config.json and config.json , while theEmbeddingDemois just a Java version of the code from the landing page of the model.Progress
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/babylon.git pull/1085/head:pull/1085$ git checkout pull/1085Update a local copy of the PR:
$ git checkout pull/1085$ git pull https://git.openjdk.org/babylon.git pull/1085/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1085View PR using the GUI difftool:
$ git pr show -t 1085Using diff file
Download this PR as a diff file:
https://git.openjdk.org/babylon/pull/1085.diff
Using Webrev
Link to Webrev Comment