Fixed passing of generation config params to VLM generate. #1180

popovaan · 2024-11-08T16:19:26Z

Fixed passing of generation config params to VLM generate().
Updated generation config params params list in update_config_from_kwargs() method.

Ticket: CVS-157050

src/python/py_utils.cpp

ilya-lavrenov · 2024-11-08T17:17:50Z

Let' submit fix to 2024.5

src/python/py_vlm_pipeline.cpp

src/python/py_utils.hpp

src/python/py_utils.cpp

Fixed passing of generation config params to VLM generate. Port of #1180

…ing_fix

src/python/py_utils.cpp

…ing_fix

src/python/py_utils.cpp

Wovchena · 2024-11-19T14:28:28Z

src/cpp/src/llm_pipeline_static.cpp

@@ -313,6 +313,9 @@ template <typename T>
 T pop_or_default(ov::AnyMap& config, const std::string& key, const T& default_value) {
    auto anyopt = pop_option(config, key);
    if (anyopt.has_value()) {
+        if (anyopt.value().empty()) {
+            return T{};


Why not default_value?
Can you explain, why it is needed now

I guess that's because you switched from exceptions to return ov::Any();, but why?

Almost any property of type list, set, or dict can potentially be empty and we should be able to accept such values. So Ilya suggested to return ov::Any() in bindings and process empty ov::Any() in utils.

Regarding usage of default_value, I tried it locally but tests, that use confing shown below failed, because it is not equivalent to creation of empty container. So I changed to T{}.

openvino.genai/tests/python_tests/test_llm_pipeline_static.py

Lines 19 to 20 in 6bb5644

'PREFILL_CONFIG': { },

'GENERATE_CONFIG': { }

but why?

yes, we need somehow to bypass information from python that a container (list, map, set) is empty, but we don't know type of this container (e.g. vector<of what type?>), while in this function we know this type T

maybe it will safer to return T{}; only for containers? because for regular type empty ov::Any is still not expected

src/python/py_utils.cpp

ilya-lavrenov · 2024-11-19T17:54:29Z

src/cpp/src/llm_pipeline_static.cpp

@@ -313,6 +313,9 @@ template <typename T>
 T pop_or_default(ov::AnyMap& config, const std::string& key, const T& default_value) {
    auto anyopt = pop_option(config, key);
    if (anyopt.has_value()) {
+        if (anyopt.value().empty()) {
+            return T{};


but why?

yes, we need somehow to bypass information from python that a container (list, map, set) is empty, but we don't know type of this container (e.g. vector<of what type?>), while in this function we know this type T

maybe it will safer to return T{}; only for containers? because for regular type empty ov::Any is still not expected

ilya-lavrenov · 2024-11-19T17:57:07Z

src/python/py_tokenizer.cpp

@@ -30,10 +30,10 @@ void init_tokenizer(py::module_& m) {
        R"(openvino_genai.Tokenizer object is used to initialize Tokenizer
           if it's located in a different path than the main model.)")

-        .def(py::init([](const std::filesystem::path& tokenizer_path, const std::map<std::string, py::object>& properties) {
+        .def(py::init([](const std::filesystem::path& tokenizer_path, const py::kwargs& kwargs) {


maybe we need to keep map for BW compatibility and add warning similar to LLMPipeline?

openvino.genai/src/python/py_llm_pipeline.cpp

Lines 110 to 116 in bef96dd

if (config.size()) {

PyErr_WarnEx(PyExc_DeprecationWarning,

"'config' parameters is deprecated, please use kwargs to pass config properties instead.",

1);

auto config_properties = pyutils::properties_to_any_map(config);

properties.insert(config_properties.begin(), config_properties.end());

}

Co-authored-by: Ilya Lavrenov <[email protected]>

popovaan marked this pull request as ready for review November 8, 2024 16:19

github-actions bot added category: visual language Visual language pipeline category: sampling Sampling / Decoding algorithms category: Python API Python API for GenAI labels Nov 8, 2024

popovaan requested review from ilya-lavrenov and Wovchena November 8, 2024 16:20

Fixed passing of generation config params to VLM generate.

0ce0ff9

ilya-lavrenov reviewed Nov 8, 2024

View reviewed changes

src/python/py_utils.cpp Outdated Show resolved Hide resolved

ilya-lavrenov modified the milestones: 2025.0, 2024.5 Nov 8, 2024

ilya-lavrenov added the bug Something isn't working label Nov 8, 2024

ilya-lavrenov self-assigned this Nov 8, 2024

ilya-lavrenov reviewed Nov 11, 2024

View reviewed changes

src/python/py_vlm_pipeline.cpp Outdated Show resolved Hide resolved

popovaan mentioned this pull request Nov 11, 2024

Fixed passing of generation config params to VLM generate. #1184

Merged

Code corrections.

104281e

Wovchena requested changes Nov 11, 2024

View reviewed changes

src/python/py_utils.hpp Outdated Show resolved Hide resolved

src/python/py_utils.cpp Show resolved Hide resolved

github-merge-queue bot pushed a commit that referenced this pull request Nov 11, 2024

Fixed passing of generation config params to VLM generate. (#1184)

a566870

Fixed passing of generation config params to VLM generate. Port of #1180

ilya-lavrenov modified the milestones: 2024.5, 2025.0 Nov 11, 2024

Merge remote-tracking branch 'upstream/master' into vlm_generate_bind…

912bc3b

…ing_fix

github-actions bot added category: text to image Text 2 image pipeline category: whisper Whisper pipeline category: GenAI C++ API Changes in GenAI C++ public headers labels Nov 16, 2024

popovaan added 3 commits November 16, 2024 13:42

Moved properties processing to common place.

383b661

Minor correction.

856db38

Merged master.

54f6df9

github-actions bot added the category: LLM LLM pipeline (stateful, static) label Nov 16, 2024

Fixed errors.

2600123

ilya-lavrenov reviewed Nov 16, 2024

View reviewed changes

src/python/py_utils.cpp Show resolved Hide resolved

src/python/py_utils.cpp Show resolved Hide resolved

src/python/py_utils.cpp Outdated Show resolved Hide resolved

src/python/py_utils.cpp Show resolved Hide resolved

src/python/py_utils.cpp Outdated Show resolved Hide resolved

Applied comments.

3cb61b4

popovaan requested review from Wovchena and ilya-lavrenov November 18, 2024 16:50

Merge remote-tracking branch 'upstream/master' into vlm_generate_bind…

7792e31

…ing_fix

Wovchena requested changes Nov 19, 2024

View reviewed changes

src/python/py_utils.cpp Outdated Show resolved Hide resolved

src/python/py_utils.cpp Outdated Show resolved Hide resolved

github-actions bot added the category: tokenizers Tokenizer class or submodule update label Nov 19, 2024

popovaan requested review from slyalin and Wovchena November 19, 2024 14:25

Applied comments.

dc19f89

Wovchena reviewed Nov 19, 2024

View reviewed changes

Fixed tests.

c928271

ilya-lavrenov reviewed Nov 19, 2024

View reviewed changes

Update src/python/py_utils.cpp

1978b67

Co-authored-by: Ilya Lavrenov <[email protected]>

github-actions bot added no-match-files category: continuous batching Continuous batching labels Nov 20, 2024

Applied comments.

446dcb6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed passing of generation config params to VLM generate. #1180

Fixed passing of generation config params to VLM generate. #1180

popovaan commented Nov 8, 2024 •

edited by andrei-kochin

Loading

ilya-lavrenov commented Nov 8, 2024

Wovchena Nov 19, 2024

popovaan Nov 19, 2024

ilya-lavrenov Nov 19, 2024 •

edited

Loading

ilya-lavrenov Nov 19, 2024 •

edited

Loading

ilya-lavrenov Nov 19, 2024

	if (config.size()) {
	PyErr_WarnEx(PyExc_DeprecationWarning,
	"'config' parameters is deprecated, please use kwargs to pass config properties instead.",
	1);
	auto config_properties = pyutils::properties_to_any_map(config);
	properties.insert(config_properties.begin(), config_properties.end());
	}

Fixed passing of generation config params to VLM generate. #1180

Are you sure you want to change the base?

Fixed passing of generation config params to VLM generate. #1180

Conversation

popovaan commented Nov 8, 2024 • edited by andrei-kochin Loading

ilya-lavrenov commented Nov 8, 2024

Wovchena Nov 19, 2024

Choose a reason for hiding this comment

popovaan Nov 19, 2024

Choose a reason for hiding this comment

ilya-lavrenov Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

ilya-lavrenov Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

ilya-lavrenov Nov 19, 2024

Choose a reason for hiding this comment

popovaan commented Nov 8, 2024 •

edited by andrei-kochin

Loading

ilya-lavrenov Nov 19, 2024 •

edited

Loading

ilya-lavrenov Nov 19, 2024 •

edited

Loading