-
Notifications
You must be signed in to change notification settings - Fork 8
Triplet Extraction Pipeline with Fine-Tuned Models #35
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
base: dev
Are you sure you want to change the base?
Conversation
pipeline models rewritten
added pipeline logic
AsphodelRem
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Есть некоторые замечания по коду. Часть из них пометил, как "на будущее" - можно поправить потом. Но некоторые лучше обсудить и исправить сейчас
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- У нас есть BatchGenerator в ragu/common/batch_generator
- Хотелось бы избавиться от магического числа 5 в строке 92 (вынести в параметры)
- Зачем нам (по крайней мере в этот момент) relation_type?
- Все отношения лучше помечать как relations, а не relationships для консистентности с остальным кодом.
- Строки 55-57 лучше переписать, пояснить, чем первый контекст отличается от второго и т.д.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Я так понимаю, здесь transformers используется для инференса ragu-lm? А почему не через vllm? Так ведь быстрее будет.
- Тут лучше промпты переиспользовать из ragu/common/prompts (это, скорее, коментарий на будущее)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не ломает ли добавление relationship_type ничего другого в коде? Если нет, то можем оставить
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это ведь уже есть в ragu/common/prompts/default_templates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это больше замечание к архитектуре классов из pipeline, но я коммент оставлю здесь, т.к. тут наглядно видно, что не нравится.
А в чем ценность создавать пайплайн вот так:
ner_client = NERClient(NER_SERVICE_BASE_URL)
nen_client = NENClient(LLM_BASE_URL)
re_client = REClient(RE_SERVICE_BASE_URL)
description_client = DescriptionClient(LLM_BASE_URL)
# Create the pipeline steps in the correct order
steps = [
NERStep(ner_client),
NENStep(nen_client),
EntityDescriptionStep(description_client),
REStep(re_client),
RelationDescriptionStep(description_client),
]
# Create the pipeline
pipeline = Pipeline(steps)
Как будто проще и логичнее сразу передавать url в pipeline, и там заводить экземпляры классов-клиентов (или steps). Вроде клиенты ни от чего, кроме url, не зависят.
Замечание на будущее, пока пусть остается так
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Так ли он нам нужен? transformers в проде обычно не используют
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Нужно подумать, как грамотно разнести зависимости по "подпакетам" (или extras). Сейчас есть два типа - дефолтный и local (pip install graph_ragu[local]).
Новый пайплайн для извлечения триплетов, основанный на работе нескольких специализированных легковесных моделей.
Ключевые изменения включают: