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

Grouped source files in GenAI src folder #1016

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

Grouped source files in GenAI src folder #1016

wants to merge 4 commits into from
+759 −755

Conversation

ilya-lavrenov
Copy link
Contributor

Created several subfolders:

  • tokenizer
  • lora
  • continuous_batching
  • LLM

@ilya-lavrenov ilya-lavrenov added this to the 2024.5 milestone Oct 18, 2024
@github-actions github-actions bot added category: text to image Text 2 image pipeline category: visual language Visual language pipeline category: continuous batching Continuous batching category: whisper Whisper pipeline category: sampling Sampling / Decoding algorithms category: speculative decoding Speculative decoding category: GHA CI based on Github actions category: cmake / build Cmake scripts category: Python API Python API for GenAI labels Oct 18, 2024
Wovchena
Wovchena reviewed Oct 18, 2024
View reviewed changes
src/python/py_llm_pipeline.cpp
@@ -391,7 +392,7 @@ std::ostream& operator << (std::ostream& stream, const GenerationResult& generat
} // namespace


PYBIND11_MODULE(py_generate_pipeline, m) {
PYBIND11_MODULE(py_llm_pipeline, m) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why don't you name the file and the module the same way cmake target is called?

src/python/py_whisper_pipeline.cpp
@@ -5,7 +5,7 @@
#include <pybind11/stl.h>
#include <pybind11/stl_bind.h>

#include "tokenizers_path.hpp"
#include "tokenizer/tokenizer_path.hpp"
#include "openvino/genai/whisper_generation_config.hpp"
#include "openvino/genai/whisper_pipeline.hpp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you modify only src folder and not include?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what are recommendations for include?

Regarding Whisper specifically, I would move WhisperGenerationConfig inside the pipeline class, as it's done for text 2 image:

openvino.genai/src/cpp/include/openvino/genai/text2image/pipeline.hpp

Lines 50 to 70 in ad2faf5

class OPENVINO_GENAI_EXPORTS Text2ImagePipeline {
public:
class OPENVINO_GENAI_EXPORTS Scheduler {
public:
enum Type {
AUTO,
LCM,
LMS_DISCRETE,
DDIM,
EULER_DISCRETE
};
static std::shared_ptr<Scheduler> from_config(const std::string& scheduler_config_path,
Type scheduler_type = AUTO);
virtual ~Scheduler();
};
struct OPENVINO_GENAI_EXPORTS GenerationConfig {
// LCM: prompt only w/o negative prompt
// SD XL: prompt2 and negative_prompt2

CC @as-suvorov

Copy link
Collaborator

@Wovchena Wovchena Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes and

  1. continuous_batching
    cache_eviction.hpp, continuous_batching_pipeline.hpp (rename to pipeline.hpp), scheduler_config.hpp
  2. llm
    llm_pipeline.hpp (rename to pipeline.hpp)
  3. lora
    lora_adapter.hpp BTW, should the files be renamed here and in src?
  4. tokenizer
    tokenizer.hpp

Copy link
Contributor Author

@ilya-lavrenov ilya-lavrenov Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

regarding 2-4 - why do we need to create a folder with a single file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Consistency. The number of symbols would stay exactly the same in both versions of include.

@ilya-lavrenov ilya-lavrenov removed this from the 2024.5 milestone Oct 31, 2024
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

At least 1 approving review is required to merge this pull request.

Assignees

@Wovchena Wovchena

Labels
category: cmake / build Cmake scripts category: continuous batching Continuous batching category: GHA CI based on Github actions category: Python API Python API for GenAI category: sampling Sampling / Decoding algorithms category: speculative decoding Speculative decoding category: text to image Text 2 image pipeline category: visual language Visual language pipeline category: whisper Whisper pipeline
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ilya-lavrenov @Wovchena