-
Notifications
You must be signed in to change notification settings - Fork 5
Statemachine #183
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
Statemachine #183
Conversation
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.
Не все просмотрел, но подсветил основные моменты
nlpf_statemachine/config.py
Outdated
TRANSACTION_TIMEOUT = config("TRANSACTION_TIMEOUT", cast=int, default=10) | ||
|
||
# MessageNames, которые заканчивают транзакцию | ||
TRANSACTION_MESSAGE_NAME_FINISH_LIST = config( |
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.
вынес константы
from nlpf_statemachine.kit import ContextManager, Form | ||
from smart_kit.configs import get_app_config | ||
|
||
app_config = get_app_config() |
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.
я бы предложил скомпилировать навык с такой версией фреймворка и проверить как оно отработает
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.
эту переменную в целом убрал за ненадобностью
STATIC_STORAGE_EVENT_1 = "STATIC_STORAGE_EVENT_1" | ||
STATIC_STORAGE_EVENT_2 = "STATIC_STORAGE_EVENT_2" | ||
STATIC_STORAGE_EVENT_3 = "STATIC_STORAGE_EVENT_3" | ||
STATIC_STORAGE_EVENT_4 = "STATIC_STORAGE_EVENT_4" |
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.
я по описанию не очень понял что это за такие эвенты. Давай с примером в smart_app_api или же в языке апи (server_action, например)
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.
Ты поменял на абстрактные, но тем самым еще более запутал все :) Там конкретные кейсы в примерах разбираются, совсем несоответствующие текущему неймингу
from nlpf_statemachine.kit import Classifier | ||
from nlpf_statemachine.models import Context, MessageToSkill, SmartEnum | ||
|
||
STATIC_PARAPHRASE = "статичный ответ" |
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.
перенес в example
from nlpf_statemachine.models import Context, MessageToSkill, SmartEnum | ||
|
||
STATIC_PARAPHRASE = "статичный ответ" | ||
NUM_TOKEN = "NUM_TOKEN" |
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.
убрал лишние константы
GetTokenRequest: Запрос в токен-сервис. | ||
""" | ||
return GetTokenRequest( | ||
payload=GetTokenPayload( |
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.
убрал
pyproject.toml
Outdated
@@ -60,6 +60,9 @@ twisted = "22.10.0" | |||
urllib3 = "1.26.18" | |||
certifi = "2023.07.22" | |||
aiokafka = "0.8.1" # TODO update poetry lock | |||
pydantic = "1.9.0" |
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.
а вот тут я бы брал сразу pydantic 2.5.*
Но для него потребуется переписать некоторые модели и наверняка даже где-то подкрутить его чтобы был не слишком strict.
Но бонусом ты получишь хороший буст к производительности. Минусы - более строгое отношение к объявлению моделей, увеличенное потребление памяти (ссылку не смог найти, но если на графиках будет рост +15% к потреблению ram - это ок).
https://docs.pydantic.dev/latest/migration/#install-pydantic-v2
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.
перенес на 2.6
from pydantic import BaseModel, Field | ||
|
||
|
||
class AnnotationsDetails(BaseModel): |
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.
я бы вынес в отд файл свою версию BaseModel и затем бы уже от нее везде во всех моделях наследовался.
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.
как минимум для версии 1.* это актуально чтобы переопределить внутри json loader и dumps на реализацию из orjson
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.
перешел на версию 2.6
from pydantic import BaseModel, Field | ||
|
||
|
||
class AppInfo(BaseModel): |
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.
а тут я бы имел какое-то скрытое свойство, типа raw_object. Чтобы ты мог к нему обратиться и вычитать необходимое поле без обновления версии фреймворка.
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.
давай отд созвонимся, я расскажу
я аппрув ставить не могу, но я посмотрел и все ост ок |
add statemachine