openvino
openvino copied to clipboard
[core] User can specify variables by ID in Model::reshape as optional paramter
Details:
- The
Model::reshape
function has been extended by additional parameter which allow user specify new variables shapes by ID. - Specify only inputs shape can be not enough in model which use Variables. Variable specify range of shape inferred form initial model. New input shape can be outside this range and then user has to specify new Variable shape.
- Provide only shapes for Parameters can be not enough after MakeStateful transformation and Variables shape has to be update also.
- The Kaldi e2e test can be updated to use new API to slave test issues.
Tickets:
- Can fix CVS-126178
@praasz, please comment on value outside Kaldi. Kaldi support was stopped a year ago. I see setting shapes for variables reasonable in the context that we started to produce stateful LLMs, but we had a way to set proper shapes for variables with ReadValue initializer.
@praasz, please comment on value outside Kaldi. Kaldi support was stopped a year ago. I see setting shapes for variables reasonable in the context that we started to produce stateful LLMs, but we had a way to set proper shapes for variables with ReadValue initializer.
Not only Kaldi models may become non-reshapable via "reshape" method if they use Variables.
E.g to reshape model after applying MakeStateful transformation, usually it's not enough to set new shapes for Parameters, rather, users need to update Variable shapes as well. MakeStateful is a common transformation, not only for Kaldi
Without these changes in "reshape" method, users need to search and update Variables manually.
About "ReadValue initializer", MakeStateful transformation doesn't create this initializer. As far as I remember, these initializers for LLMs were manually created.
@praasz, please comment on value outside Kaldi. Kaldi support was stopped a year ago. I see setting shapes for variables reasonable in the context that we started to produce stateful LLMs, but we had a way to set proper shapes for variables with ReadValue initializer.
The plan was to update description with more or less @itikhono mention. The e2e failures on KALDI models just unhide the issue with reshape when model use variables. Will do it when push all changes.
are you ok with merge it, or you have specific change requests that should wait for Pawel fixes?
@itikhono , @jane-intel could you review the PR?
General questions:
- Why did we pick to use strings to identify variables? We could've used indices or instances instead. It looks like name is user friendly to identify it.
The indices could be used but I think is more enigmatic for user and I'm not sure if will easy identify it in model if required variable will be always at same position.
The instances is like some kind of ID (address) which value is even more not friendly. The get variables by instance there is function get_variables()
. Get and use instance will allow to modify shapes before input reshape which can result with error in validation:
ov::pass::Manager ssr_manager;
ssr_manager.register_pass<ov::pass::SmartReshape>();
// Here the old shapes for inputs and variables has to be used, if variables reshaped only then could be error here on validation.
ssr_manager.run_passes(shared_from_this());
// Here new shapes for inputs and variables can be applied and validated.
reshape_only(new_param_shapes, new_vars_shapes);
But If you got ideas how used no string ids but indices or instances which will be useful I can adjust implementation. I would recommend to limit the ID
type to minimum to not produce to many reshape overloads.
- Why do we add 4 methods instead of adding optional parameters to existing ones?
Is done to not create the default map on each call (is internally created only once and reference used instead). If required it can be changed to use default parameter instead.
Do we need an architecture review? We are changing the public API here, it is required.
Will check and can wait with merge when changes will be approved.
This PR will be closed in a week because of 2 weeks of no activity.