Skip to content

[Core][Ref][CPU] Multinomial with RandomUniform samples #31034

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

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

PiotrKrzem
Copy link
Contributor

@PiotrKrzem PiotrKrzem commented Jun 18, 2025

STATUS:

ON HOLD, waiting for info about opset version (discussion below) + priority shift

Details:

  • Add RU as a base source of randomness for samples

Tickets:

  • 126095

@github-actions github-actions bot added category: Core OpenVINO Core (aka ngraph) category: CPU OpenVINO CPU plugin category: transformations OpenVINO Runtime library - Transformations category: CPP API OpenVINO CPP API bindings labels Jun 18, 2025
@PiotrKrzem PiotrKrzem marked this pull request as ready for review July 3, 2025 15:27
@PiotrKrzem PiotrKrzem requested review from a team as code owners July 3, 2025 15:27
@PiotrKrzem PiotrKrzem requested review from itikhono and removed request for a team July 3, 2025 15:27
@PiotrKrzem PiotrKrzem requested a review from a team as a code owner July 4, 2025 00:09
@github-actions github-actions bot added the category: TEMPLATE OpenVINO Template plugin label Jul 4, 2025
@PiotrKrzem PiotrKrzem requested review from a team as code owners July 8, 2025 08:51
@github-actions github-actions bot added category: IE Tests OpenVINO Test: plugins and common category: TFL FE OpenVINO TensorFlow Lite FrontEnd category: JS API OpenVino JS API Bindings labels Jul 8, 2025
@github-actions github-actions bot removed the category: TFL FE OpenVINO TensorFlow Lite FrontEnd label Jul 8, 2025
@github-actions github-actions bot removed the category: JS API OpenVino JS API Bindings label Jul 8, 2025
@github-actions github-actions bot removed the category: IE Tests OpenVINO Test: plugins and common label Jul 8, 2025
@PiotrKrzem PiotrKrzem requested a review from a team as a code owner July 11, 2025 23:31
@github-actions github-actions bot added the category: ONNX FE OpenVINO ONNX FrontEnd label Jul 11, 2025
Copy link
Contributor

@mitruska mitruska left a comment

Choose a reason for hiding this comment

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

Goal:
The goal of this PR is to reuse RandomUniform logic as a source of random samples of Multinomial operation to align with frameworks random generators.

Changes Review:
This PR extends existing Multinomial-13 op with new attribute (for PhiloxAlignment) and input (for random_samples). As long as it's backward compatible update, this approach can be reviewed within the same op version, but there should be a common agreement with plugins on that.
@maxnick @sshlyapn @birozsolt

From my understanding this new input is not supposed to be filled by user or by the values from an original model, but by RandomUniform operation, which is inserted during the proposed common transformation, replacing every Multinomial of originally two inputs, with Multinomial(..., num_samples=RandomUniform), where the output of RU is the new third random_samples.

Risks:
Introducing the MultinomialRandomUniformFusion as a common transformation is an interesting idea, allowing for easy update of the Multinomial logic, without significant changes in the plugin code. But it will affect all already supported models with Multinomial, and not only CPU plugin. Currently it looks a bit like work-around, than a standard approach.

Question: (@PiotrKrzem @nshchego @itikhono)
Can we eloborate more on the right direction here, and review the proposed transformation vs usage of RandomUniform kernel inside the plugin Multinomial kernel explicitly?

#include "openvino/op/reshape.hpp"
#include "openvino/op/shape_of.hpp"
#include "openvino/pass/pattern/op/wrap_type.hpp"
#include "transformations/common_optimizations/mul_fake_quantize_fusion.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove redundant includes:

Suggested change
#include "transformations/common_optimizations/mul_fake_quantize_fusion.hpp"


new_multinomial->set_friendly_name(multinomial->get_friendly_name());
ov::copy_runtime_info(multinomial, {random_uniform, new_multinomial});
ov::replace_node(multinomial, new_multinomial);
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +42 to +43
const uint64_t op_seed = 0,
const PhiloxAlignment alignment = PhiloxAlignment::TENSORFLOW);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this alignment attribute has effect only if the replacement transformation is applied and random_samples input is provided.

Comment on lines +153 to +168
if (m_provided_random_samples) {
const auto& random_samples_shape = getParentEdgeAt(RANDOM_SAMPLES_PORT)->getMemory().getStaticDims();

if (random_samples_shape.size() != 1) {
THROW_CPU_NODE_ERR("has incompatible 'random_samples' shape ",
PartialShape(random_samples_shape),
". Only 1D tensors are allowed.");
}

if (random_samples_shape[0] != m_output_elements_count) {
THROW_CPU_NODE_ERR(
"has incompatible 'random_samples' shape ",
PartialShape(random_samples_shape),
". The total elements of this input should be equal to the total number of elements in the output.");
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Those checks covered by Multinomial shape_infer. Is there a need to validate it again here?

@@ -206,20 +242,23 @@ void Multinomial::execute_convert_type() {
});
}

// TODO RandomUniform - should use RandomUniform kernel to match other frameworks' seed results
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible and more efficient to use the RandomUniform cpu kernel explicitly by Multinomial, instead of providing it as input?

Comment on lines +101 to +121
if (random_samples == nullptr) {
const T zero = 0;
const T one = 1;
const ov::Shape output_shape_shape{output_shape.size()};
const std::vector<uint64_t> output_shape_u64(output_shape.begin(), output_shape.end());
const std::pair<uint64_t, uint64_t> initial_state(0, 0);
random_uniform(output_shape_u64.data(),
reinterpret_cast<const char*>(&zero),
reinterpret_cast<const char*>(&one),
reinterpret_cast<char*>(uniform_samples.data()),
output_shape_shape,
ov::element::from<T>(),
global_seed,
op_seed,
initial_state);
} else {
const auto random_samples_count = shape_size<Shape>(random_samples_shape);
OPENVINO_ASSERT(random_samples_count == total_output_elements_count,
"Multinomial random_samples count is not equal to output_shape size");
uniform_samples.assign(random_samples, random_samples + random_samples_count);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

From the reference implementation (Template plugin) tests perspective, if the replacement transformation is always applied, the random_samples == nullptr path is not tested at all.

Comment on lines +245 to +259
if (!m_provided_random_samples) {
std::mt19937 gen;
if (m_global_seed == 0 && m_op_seed == 0) {
gen.seed(std::time(nullptr));
} else {
std::seed_seq seed{m_global_seed, m_op_seed};
gen.seed(seed);
}
const auto gen_max = static_cast<float>(std::mt19937::max());
std::generate(m_random_samples.begin(), m_random_samples.end(), [&]() {
return static_cast<P>(static_cast<float>(gen()) / gen_max);
});
} else {
std::seed_seq seed{m_global_seed, m_op_seed};
gen.seed(seed);
const auto* samples = getSrcDataAtPortAs<const P>(RANDOM_SAMPLES_PORT);
m_random_samples.assign(samples, samples + m_output_elements_count);
Copy link
Contributor

Choose a reason for hiding this comment

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

If the Multinomial replacement transformation is always applied as common transformation, the if (!m_provided_random_samples) path is not tested for CPU by any tests.


private:
ov::element::Type_t m_convert_type;
bool m_with_replacement;
bool m_log_probs;
uint64_t m_global_seed;
uint64_t m_op_seed;
op::PhiloxAlignment m_alignment;
Copy link
Contributor

Choose a reason for hiding this comment

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

This new attribute is actually used only by the introduced transformation, but not inside the Multinomial logic (ref or CPU).

@PiotrKrzem PiotrKrzem added do not merge no_stale Do not mark as stale labels Jul 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: Core OpenVINO Core (aka ngraph) category: CPP API OpenVINO CPP API bindings category: CPU OpenVINO CPU plugin category: ONNX FE OpenVINO ONNX FrontEnd category: TEMPLATE OpenVINO Template plugin category: transformations OpenVINO Runtime library - Transformations do not merge no_stale Do not mark as stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants