-
Notifications
You must be signed in to change notification settings - Fork 99
Задача 2 #167
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: main
Are you sure you want to change the base?
Задача 2 #167
Conversation
| "Authorization": f"Bearer {api_token}", | ||
| "Content-Type": "application/json" | ||
| } | ||
| self.inputs = [ |
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.
это можно просто не в init написать, а в классе над методом, потому что это все равно одинаково для всех объектов
| response = requests.post(f"{self.api_url}@cf/meta/llama-3-8b-instruct", headers=self.headers, json=data) | ||
|
|
||
|
|
||
| if response.status_code != 200: |
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.
- Нужно поднимать ошибку через метод raise_for_status
- Успешный код ответа это не только 200, но и любой от 200 до 299
- print в проектах не искользуют, надо logging или loguru
- Если случилась какая-то ошибка её просто всегда прокидываем на самый верх и ловим в fastapi через exception handler
| return None | ||
|
|
||
| result = response.json() | ||
| return result.get("result", {}).get("response", "⚠️ Нет ответа в JSON") No newline at end of file |
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.
тут опять же ошибку надо кидать если ниче нет, а не рандомный текст
| MASTER_KEY = os.getenv('MASTER_KEY') | ||
| storage = CloudJSONStorage(bin_id=BIN_ID, master_key=MASTER_KEY) | ||
|
|
||
| API_TOKEN_AI = os.getenv('API_TOKEN_AI') |
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.
- Вместо os щас моднее использовать starlette.config для работы с переменными окружения и pydantic.base_settings для класса конфига
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.
Также все переменные в отдельном файле cpnfig.py пиши
| @app.put("/tasks/{task_id}") | ||
| def update_task(task_id: int): | ||
| pass | ||
| def update_task(task_id: int, task_data: TaskCreate = Body(...)): |
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.
Вместо Body и прочей шляпы от фастапи используй pydantic модели
| from pydantic import BaseModel | ||
|
|
||
| class TaskCreate(BaseModel): | ||
| title: str |
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.
нету параметров валидации никаких (например минимальная длина строки)
| def delete_task(task_id: int): | ||
| pass | ||
| is_delete = storage.delete_task(task_id) | ||
| if is_delete: return True |
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.
опять же, никаких true false, просто выкидывай ошибку в storage.delete_task, если не получилось что-то. мы не на языке go пишем, где исключения не опрокидываются. внешняя функция должна знать как можно меньше о той, которую использует внутри себя
| self.next_task = 1 | ||
|
|
||
|
|
||
| def list_tasks(self): |
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.
нигде не типизируешь, что возвращается из функции
| status: bool | ||
|
|
||
| class Task(TaskCreate): | ||
| id: int No newline at end of file |
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.
ТЗ внимательно читай - У задачи должны быть параметры: id, название, статус задачи.
| return self.tasks | ||
|
|
||
| def create_task(self, task_data): | ||
| task = {'id': self.next_task, **task_data} |
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 возвращай, никаких диктов
LilChichaaa
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.
не все подпункты ТЗ выполнил, пересмотри. где текст в .md про хранение состояния?
| * можно создать резервные копии | ||
|
|
||
| ## избавились ли мы таким способом от хранения состояния или нет? | ||
| * Нет, мы не избавились от хранения состояния, мы просто перенесли его из оперативной памяти на файл. |
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.
да, но сервис стал stateless , а не stateful
| * Нет, мы не избавились от хранения состояния, мы просто перенесли его из оперативной памяти на файл. | ||
|
|
||
| ## где еще можно хранить задачи и какие есть преимущества и недостатки этих подходов? | ||
| ### реляционные бд |
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.
ну и нереляционные тоже
| @@ -0,0 +1,26 @@ | |||
| import requests | |||
|
|
|||
| class BaseHTTPClient: | |||
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.
модуль abc нужен , чтоб создать абстрактный класс и абстрактные методы
| response_text = result["result"]["response"] | ||
|
|
||
| if not response_text: | ||
| raise ValueError("Пустой или некорректный ответ от CloudFlare AI") |
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.
можно своё кастомное исключение создать и его выбрасывать
| ) | ||
|
|
||
| def generate_answer(self, task: str) -> str: | ||
| data = {"messages": self.inputs + [{"role": "user", "content": task}]} |
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 модель перегонять сразу после получения http ответа
|
|
||
| @app.get("/tasks") | ||
|
|
||
| @app.exception_handler(ValueError) |
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.
напрямую к приложению кстати не подключаем ничего. создаём Router и все коннектим к нему - эндпоинты и хендлеры
|
|
||
|
|
||
|
|
||
| @app.get("/tasks", response_model=List[Task]) |
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.
вместо typing.List типизируй через обычный list, это устаревшее
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.
ну и вообще никаких общих типов в response_model. Только pydantic модели. если возвращается список, значит делаешь ещё одну модель TaskList с полем-списком объектов Task и возвращаешь его
| class IsMemoryStorage: | ||
|
|
||
| def __init__(self): | ||
| self.tasks = [] |
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.
вообще можно было бы ещё общий интерфейс Storage сделать кстати
| self._save(tasks) | ||
| return task | ||
|
|
||
| class CloudJSONStorage(BaseHTTPClient, JSONStorage): |
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.
тут конечно лучше бы разделить - сделать два класса - класс хранилища задач и класс http клиента. и класс хранилища внутри себя использует класс http клиента для работы с облаком. а так получается у тебя нарушения принципа S из solid - класс сразу за две задачи отвечает, работу с задачами и работу с http
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.
короче чаще всего множественное наследование - плохо, если это не миксины
LilChichaaa
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.
No description provided.