Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Image generation: added TorchGenerator and rng_seed #1379

Merged
merged 2 commits into from
Dec 16, 2024
Merged

Image generation: added TorchGenerator and rng_seed #1379

merged 2 commits into from
Dec 16, 2024

Conversation

ilya-lavrenov
Copy link
Contributor

@ilya-lavrenov ilya-lavrenov commented Dec 13, 2024 •
edited
Loading

  • Added TorchGenerator which wraps torch.Generator. It throws an exception is torch is not available.
  • Added rng_seed parameter to ImageGenerationConfig which has lower priority compared with generator when they both are specified to generate() or ImageGenerationConfig::update_generation_config

@github-actions github-actions bot added category: WWB PR changes WWB category: text to image Text 2 image pipeline category: Python API Python API for GenAI category: samples GenAI samples labels Dec 13, 2024
@ilya-lavrenov ilya-lavrenov marked this pull request as ready for review December 13, 2024 10:50
@ilya-lavrenov ilya-lavrenov added this to the 2025.0 milestone Dec 13, 2024
@ilya-lavrenov ilya-lavrenov force-pushed the torch-generator branch 2 times, most recently from 2947845 to 66a506c Compare December 13, 2024 10:58
Wovchena
Wovchena reviewed Dec 13, 2024
View reviewed changes
samples/python/image_generation/README.md Outdated
@@ -20,6 +20,10 @@ Users can change the sample code and play with the following generation paramete
- Apply multiple different LoRA adapters and mix them with different blending coefficients
- (Image to image and inpainting) Play with `strength` parameter to control how initial image is noised and reduce number of inference steps

> [!NOTE]
> All samples use `openvino_genai.TorchGenerator` as an argument to `generate` call to align random numbers generation with Diffusers library where `torch.Generator` is used underhood.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
> All samples use `openvino_genai.TorchGenerator` as an argument to `generate` call to align random numbers generation with Diffusers library where `torch.Generator` is used underhood.
> All Python samples use `openvino_genai.TorchGenerator` as an argument to `generate` call to align random numbers generation with Diffusers library where `torch.Generator` is used underhood.

It reads "every existing genai sample" otherwise, including C++.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this README.md is in samples/python/image_generation

Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem is that we are pretty good at aligning C++ and Python. One may assume that C++ readme states the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how C++ samples can use torch.Generator ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Before I red the changes my assumption was that you had copied full torch generator to genai. That would enable it for C++

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, please check

src/python/py_image_generation_pipelines.cpp Outdated
throw std::runtime_error("Expected a NumPy array with dtype float32");
}

return ov::Tensor(ov::element::f32, shape, numpy_array.mutable_data());
Copy link
Collaborator

Choose a reason for hiding this comment

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

So you keep m_torch_tensor as a member to keep this returned tensor valid. Calling randn_tensor() again would override m_torch_tensor and loose the previous data. You probably need that trick with an allocator here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, updated

src/python/py_image_generation_pipelines.cpp Outdated
m_float32 = m_torch.attr("float32");
} catch (const py::error_already_set& e) {
if (e.matches(PyExc_ModuleNotFoundError)) {
PyErr_WarnEx(PyExc_ImportWarning, "'torch' module is not installed. Random generation will fall back to 'CppStdGenerator'", 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Printing anything from a library isn't really welcome (at least for me). It's an application's (sample's in our case) responsibility to communicate with a user.
My proposal is the following. TorchGenerator should throw if torch is missing. Introduce a function in samples that picks an available generator and prints this warning.

I have another concern. Why do we need a C++ wrapper over python in the first place. Samples themselves could define a thin generator in python similar to this TorchGenerator because there's no benefit in jumping between C++ and Python multiple times. That implementation would also serve as an example generator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My proposal is the following. TorchGenerator should throw if torch is missing

Having slept on it, I agree that throwing exception is better idea.

@github-actions github-actions bot added category: sampling Sampling / Decoding algorithms category: GenAI C++ API Changes in GenAI C++ public headers labels Dec 16, 2024
@ilya-lavrenov ilya-lavrenov changed the title Image generation: added TorchGenerator Image generation: added TorchGenerator and rng_seed Dec 16, 2024
@ilya-lavrenov ilya-lavrenov added this pull request to the merge queue Dec 16, 2024
@eaidova eaidova mentioned this pull request Dec 16, 2024
Merged
Merged via the queue into openvinotoolkit:master with commit 7548c4c Dec 16, 2024
59 checks passed
@ilya-lavrenov ilya-lavrenov deleted the torch-generator branch December 17, 2024 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wovchena Wovchena Wovchena left review comments

@yury-gorbachev yury-gorbachev yury-gorbachev left review comments

@likholat likholat likholat approved these changes

@eaidova eaidova eaidova approved these changes

@AlexKoff88 AlexKoff88 AlexKoff88 approved these changes

Assignees

@Wovchena Wovchena

@AlexKoff88 AlexKoff88

@eaidova eaidova

@likholat likholat

@popovaan popovaan

Labels
category: GenAI C++ API Changes in GenAI C++ public headers category: Python API Python API for GenAI category: samples GenAI samples category: sampling Sampling / Decoding algorithms category: text to image Text 2 image pipeline category: WWB PR changes WWB no-match-files
Projects
None yet
Milestone
2025.0
Development

Successfully merging this pull request may close these issues.
7 participants
@ilya-lavrenov @yury-gorbachev @Wovchena @AlexKoff88 @eaidova @likholat @popovaan